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

feat: disable travel planner flight filter by default #242

Merged
merged 8 commits into from
Feb 28, 2024

Conversation

jonasbrunvoll
Copy link
Contributor

Temporary solution for task https://github.com/orgs/AtB-AS/projects/10/views/13?pane=issue&itemId=51482158

Background

NFK have requested that flights are not part of the default transport methods in travel planner web. It should still be possible to enable for the users, but not selected as default.

This is default also in app and at Entur.

Illustrations

screenshot Screenshot 2024-02-28 at 12 36 11

Proposed solution

Disabling the travel planner flight filter for for all organisations until a more sophisticated solution where each organisation can configure filter preferences.

Acceptance Criteria

  • Flight filter is unchecked by default.
  • Searching after trips with flight can still be performed by checking the flight filter.

Copy link

vercel bot commented Feb 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
planner-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 28, 2024 2:36pm

useEffect(() => {
setDefaultFilters(
transportModeFilter
?.filter((filter) => filter.icon.transportMode !== 'air')
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this always filter out air, regardless if the user has manually enabled it to see flights?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the user can manually set check the air filter after the initial search.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, because the transportModeFilter never changes, which means that the useEffect only runs once. In theory at least.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about when refreshing the page / opening a link with airplane filter enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As of now, refreshing the page will reset the filters to default where flight is unchecked. Opening a link with flight filter checked is works as it should, where both trip with and without flights are displayed correctly. But, when navigating back to search results from detailed view, the filters are reset again

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like the URL state is not prioritized and that this would override it 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Navigating back causes the will reset the trip query to not contain a transport mode filter, causing the useEffect to run again is my guess. Using setValuesWithLoading would remediate that, wouldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

My idea was that something like this would work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Scratch that, everything didn't work.

I think the approach with using onTransportFilterChanged might be a good solution.

Also need to make a change with "All", something like below. Setting the filter to all of the options instead of null. (Need to handle when data is undefined or null or whatever).

onChange={(event) => {
onChangeWrapper(event.target.checked ? null : []);
}}

            onChange={(event) => {
              onChangeWrapper(
                event.target.checked ? data?.map((option) => option.id) : [],
              );
            }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey👍

useEffect(() => {
if (tripQuery.transportModeFilter === null)
onTransportFilterChanged(
transportModeFilter
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this logic be added directly to the onTransportFilterChanged and this hook can be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure that is possible, as the transportModeFilter initially is null before the useSWRImmutable() have returned the results.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are also other reasons that I did not see before.

But this should be fixed:
ESLint: React Hook useEffect has missing dependencies: 'onTransportFilterChanged' and 'tripQuery.transportModeFilter'. Either include them or remove the dependency array.(react-hooks/exhaustive-deps)

@jonasbrunvoll jonasbrunvoll merged commit 196dd79 into main Feb 28, 2024
5 of 6 checks passed
@jonasbrunvoll jonasbrunvoll deleted the jonas/uncheck_air_by_default branch February 28, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants