Skip to content

remove time spent active #1389

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

josh-willis-arcadis
Copy link
Contributor

Description:

Removes the time spent active information in the trip preview layout

PR Checklist:

  • Does the code follow accessibility standards (WCAG 2.1 AA Compliant)?
  • Are all languages supported (Internationalization/Localization)?
  • Are appropriate Typescript types implemented?

Sorry, something went wrong.

<TripDetails className="percy-hide" itinerary={itinerary} />
<TripDetails
className="percy-hide"
displayTimeActive={false}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be configurable/displayed by default!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. I forgot to pull the config value.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to be a bother but since this is such a simple feature might it be better to disable this via css? I think a prop to hide a single row is clutter. Could we not just patch otp-ui to add a class name?

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dupe of #1394?

@josh-willis-arcadis
Copy link
Contributor Author

Definitely a dupe of #1394. Which one do we like better? @miles-grant-ibigroup @daniel-heppner-ibigroup

@daniel-heppner-ibigroup
Copy link
Contributor

Sorry but I like mine better because it passes in the config items with the other config stuff in the connect function. However I'm kinda with Miles that we should just do this with a CSS class.

@josh-willis-arcadis
Copy link
Contributor Author

@daniel-heppner-ibigroup You have a cleaner solution so lets use your PR. The TripDetails component already supports the prop displayTimeActive so I think we should just leave it as is.

@danielhep
Copy link
Contributor

I kinda feel like if we're going to have a config item for this, we should just go all the way and make it an array where you can specify all the items that get displayed in Trip Details. Like tripDetailsItems: ['fare', 'co2', 'timeActive', 'duration']

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@josh-willis-arcadis
Copy link
Contributor Author

@daniel-heppner-ibigroup that is a better long term solution. Can we use this as a stopgap for now and clean up the props/trip-details component in a future PR?

@miles-grant-ibigroup
Copy link
Collaborator

@daniel-heppner-ibigroup that is a better long term solution. Can we use this as a stopgap for now and clean up the props/trip-details component in a future PR?

Can we do the CSS approach instead? I really want to stop using these props

@josh-willis-arcadis
Copy link
Contributor Author

yeah I will implement css. like this? @miles-grant-ibigroup

            <TripDetails
              className={`percy-hide${
                displayTimeActive ? ' show-time-active' : ''
              }`}
              itinerary={itinerary}
            />

@miles-grant-ibigroup
Copy link
Collaborator

yeah I will implement css. like this? @miles-grant-ibigroup

            <TripDetails
              className={`percy-hide${
                displayTimeActive ? ' show-time-active' : ''
              }`}
              itinerary={itinerary}
            />

Eeek! No! I meant more adding a time-active class to the time active row and removing the displayTimeActive prop entirely

@josh-willis-arcadis
Copy link
Contributor Author

closing in favor of opentripplanner/otp-ui#841

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants