graphhopper / graphhopper-maps

GraphHopper Maps - Open Source Route Planner UI
https://graphhopper.com/maps/
Apache License 2.0
94 stars 34 forks source link

Refactored settings system - Improved GPX export #365

Closed SachsenspieltCoding closed 1 year ago

SachsenspieltCoding commented 1 year ago

fixes #315 extends #260 and 4fa516f

Refactored settings system - Improved GPX export

This PR does some refactoring of the settings system.

New SettingsContext

Currently only the showDistanceInMiles is provided globally with the corresponding context. This will now be extended to provide the whole settings store, making it easier to add new settings options.

Updated SettingsStore.ts and related actions

To reduce the number of actions needed to update the settings, it is now possible to update the settings with a single UpdateSettings action. When dispatching, you only need to provide a Partial<Settings> object with all the changes you want to merge into the settings store.

Improved GPX export

In 4fa516f the GPX export was stripped down a bit. You can now re-enable the rte and wpt data in the settings menu. grafik

Improved styling in SettingsBox.tsx and new SettingsToggle

To reduce boilerplate, a new SettingsToggle has been added to the SettingsBox. It also simplifies the creation of new toggles in the Settings menu.

[!WARNING] Because the styling is different, branches like navi will need to adapt the new SettingsToggle component.

Further questions

As this is more of a larger PR, I am open to discussion and change requests.

SachsenspieltCoding commented 1 year ago

@karussell All requested changes should be implemented now

ratrun commented 1 year ago

I tested this branch and functionality wise everything looks good to me :satisfied:. But I would like to discuss if there are possibilities to improve the user experience. What I mean is that only few users by chance might find out how to use it. The old maps implementation popped up a dialog after the user pressed the GPX export button and there the options could be modified if needed. So this was pretty obvious. Maybe most users wondered what this features was about, but as long as he/she didn't change the default it was fine. Now that this feature has moved to the settings there is only little chance that they will find it out. I currently see three possibilities:

  1. Move the settings to a popup such that the old functionality is re-implemented
  2. Somehow automatically point the user to the possibility of modifying the export settings when pressing the GPX button for the first time. But then the user would probably need to press the GPX button twice for the first time. And as far as I know the application does not set any cookies, which is a good thing, this would be needed to be done over and over again on each new usage of the web site
  3. Deliberately keep it as implemented in the branch, consider it as a power user feature and hope that these will find it out somehow
ratrun commented 1 year ago

Besides the more important remarks on the usability above I have some minor suggestions for optimization of the texts:

karussell commented 1 year ago

Deliberately keep it as implemented in the branch, consider it as a power user feature and hope that these will find it out somehow

Yes, this is IMO a power user feature. As our UI is not really complicated the settings button is obvious?

I would remove the text (trk), (rte), and (wpt) - as this is too technical

Interesting. I would have thought that when people use GPX they'd know these things?

@karussell All requested changes should be implemented now

@SachsenspieltCoding I know this is a bit late and maybe we should do it in a follow up PR but what do you think about more compact settings? Like this (probably more beautiful ;)):

image

SachsenspieltCoding commented 1 year ago

I completely agree with @karussell. The dialogue box may be a more obvious way to do it, but it would not only require more code and styling, it would also make the UX more complex than just clicking the download button. From my understanding of how GPX works, most of the time the <trk> is all you need. I think if you need the other tags (rte, wpt) you would probably be smart enough to open the settings menu. So IMO the options in the general settings are not that dramatic.

As for the other idea of simplifying the UI with checkboxes: I'll look into it as it sounds like a better solution to reduce the height of the settings, especially on mobile when more settings are coming in (like with the navi branch).

SachsenspieltCoding commented 1 year ago

@karussell Like this?

grafik

karussell commented 1 year ago

Thank you!