Skip to content
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

Deselect all submodes #1265

Merged
merged 15 commits into from
Oct 22, 2024
Merged

Deselect all submodes #1265

merged 15 commits into from
Oct 22, 2024

Conversation

josh-willis-arcadis
Copy link
Contributor

@josh-willis-arcadis josh-willis-arcadis commented Sep 3, 2024

Description:
When the user deselects all transport submode options(transit and rental), the mode should be disabled. This is reflected in the URL query params. When the mode is enabled after being disabled by deselecting all transport submode options, the defaults are used.

Blocked by opentripplanner/otp-ui#782

PR Checklist:

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

Copy link
Contributor

@daniel-heppner-ibigroup daniel-heppner-ibigroup left a comment

Choose a reason for hiding this comment

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

first round of thoughts on this here.
In general we should not be repeating work to find the mode buttons that we already have access to elsewhere in the application.

onToggleModeButton={setModeButton(
enabledModeButtons,
onSettingsUpdate(setQueryParam)
onSettingsUpdate={onAdvancedModeSubsettingsUpdate(
Copy link
Contributor

Choose a reason for hiding this comment

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

What I am uncomfortable with here is that the advanced settings panel already has the mode button that corresponds with the mode setting that is being updated. I don't think we should do a bunch of work to get that mode button item again. It will require an update to the OTP UI component but I think there's a better way to go about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, maybe it would be possible to do this change almost entirely within OTP UI? Could we just add a prop for transportModesDisableCascade? Weird name idk but something like that

@josh-willis-arcadis josh-willis-arcadis added the BLOCKED Blocked (waiting on another PR to be merged) label Sep 10, 2024
Copy link
Contributor

@daniel-heppner-ibigroup daniel-heppner-ibigroup left a comment

Choose a reason for hiding this comment

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

first round, I'll finish reviewing when OTP UI is merged

lib/components/form/advanced-settings-panel.tsx Outdated Show resolved Hide resolved
lib/components/form/util.tsx Outdated Show resolved Hide resolved
lib/components/form/util.tsx Outdated Show resolved Hide resolved
Base automatically changed from master-trip-form-update to dev September 25, 2024 17:27
@josh-willis-arcadis josh-willis-arcadis removed the BLOCKED Blocked (waiting on another PR to be merged) label Sep 25, 2024
@josh-willis-arcadis josh-willis-arcadis added the BLOCKED Blocked (waiting on another PR to be merged) label Sep 26, 2024
@josh-willis-arcadis josh-willis-arcadis removed the BLOCKED Blocked (waiting on another PR to be merged) label Oct 7, 2024
lib/components/form/advanced-settings-panel.tsx Outdated Show resolved Hide resolved
lib/components/form/util.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@daniel-heppner-ibigroup daniel-heppner-ibigroup left a comment

Choose a reason for hiding this comment

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

awesome! this is clean!

lib/components/form/advanced-settings-panel.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@amy-corson-ibigroup amy-corson-ibigroup left a comment

Choose a reason for hiding this comment

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

Curious what you both think of this setTimeout idea @daniel-heppner-ibigroup @josh-willis-arcadis. We can also address in a follow up PR if need be

Copy link
Contributor

@amy-corson-ibigroup amy-corson-ibigroup left a comment

Choose a reason for hiding this comment

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

Looking really good as-is, we can move the conversation about potential setTimeout to the OTP-UI PR! Thank you so much for your work on this!! :) Also just flagging daniel's note about the +1 line on 172.

@josh-willis-arcadis josh-willis-arcadis merged commit cab7c04 into dev Oct 22, 2024
9 checks passed
@josh-willis-arcadis josh-willis-arcadis deleted the deselect-all-submodes branch October 22, 2024 18:49
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.

3 participants