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.

Sign up for GitHub

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

Jump to bottom

Refactor resource modal - v2 #463

Merged
merged 23 commits into from
Aug 7, 2024
Merged

Refactor resource modal - v2 #463

merged 23 commits into from
Aug 7, 2024

Conversation

ravicodelabs
Copy link
Contributor

@ravicodelabs ravicodelabs commented Jun 25, 2024 •
edited
Loading

This PR replaces the prior PR #430, and addresses issue #422.

The desktop and mobile resource selection modals have been refactored as follows:

  • Removed the prior mobile approach to show/hide the resource menu and replaced with the desktop approach. Previously, toggleResourceMenu was used with state isResourceMenuShown. Now, setToolbarModal is used and is consistent with how other toolbar modals are shown/hidden.
  • Removed the two actions TOGGLE_RESOURCE_TYPE and CHANGE_RESOURCE_TYPE in favor of a new action called SET_SELECTED_PLACE SET_RESOURCE_TYPE, which maintains the same functionality, but is named consistent with other actions in actions.js and is used for both desktop and mobile.
  • Renamed the mobile component ResourceMenu to MobileChooseResource, and the analogous desktop component to DesktopChooseResource, and moved both to src/components/ChooseResource.

Also, some miscellaneous clean up was done, in particular:

  • Remove no longer used TypeToggle.js component

Related Issues: #422, #401

@ravicodelabs
Prettier formatting
9d08883
@ravicodelabs
Refactor resource menu show/hide
5577dac 
The mobile resource menu modal shown state was being handled with
its own action, TOGGLE_RESOURCE_MENU. Here we retire that action,
and instead use SET_TOOLBAR_MODAL, which is consistent with how
other toolbar modals' shown state is being handled on both mobile
and desktop.
@ravicodelabs
Refactor of resource selection
8cd8a9b 
Consolidate behavior of CHANGE_RESOURCE_TYPE and TOGGLE_RESOURCE_TYPE
actions into one new action, SET_RESOURCE_TYPE.
@ravicodelabs
Remove unused past component
dc00d3d
@ravicodelabs
Reorganize resource selection components
383c56c 
Use consistent naming for mobile and desktop resource menu
components, and bring to one location.
This was referenced Jun 25, 2024
Closed
Closed
@ravicodelabs ravicodelabs marked this pull request as draft June 25, 2024 20:43
@ravicodelabs ravicodelabs marked this pull request as ready for review June 26, 2024 15:22
@tomporvaz tomporvaz linked an issue Jul 9, 2024 that may be closed by this pull request
Consolidate how the resource modal is rendered on mobile and desktop to potentially use the approach built in PR #332 #422
Closed
@gcardonag
Copy link
Contributor

Just a quick comment on the PR description, is it fair to assume that the second bullet meant to say SET_RESOURCE_TYPE instead of SET_SELECTED_PLACE? I believe that's the intent given the changes but wanted to confirm.

@gcardonag
Copy link
Contributor

@ravicodelabs - These changes all look great, thank you for bringing them in! Before approving, there was one thing we can talk about on today's meetup with the wider group. Now that the resource menu on mobile is showing, I think it would be good to run dispatch(setToolbarModal(TOOLBAR_MODAL_NONE)) when clicking on one of the resource menu options on mobile. My reasoning for doing so differently than on Desktop is that on the desktop you can see the rest of the map and therefore the side-effect of clicking the resource selector. On mobile, the selector takes up most of the screen, so it isn't as clear as a user if my action caused an effect on the map. I believe closing the modal will serve as a response to the click and bring visibility to the pins for the new resource type selection.

@ravicodelabs
Copy link
Contributor Author

Just a quick comment on the PR description, is it fair to assume that the second bullet meant to say SET_RESOURCE_TYPE instead of SET_SELECTED_PLACE? I believe that's the intent given the changes but wanted to confirm.

Yes, this assumption is correct, I've edited the prior comment.

@ravicodelabs
Copy link
Contributor Author

ravicodelabs commented Jul 30, 2024 •
edited
Loading

... Now that the resource menu on mobile is showing, I think it would be good to run dispatch(setToolbarModal(TOOLBAR_MODAL_NONE)) when clicking on one of the resource menu options on mobile...

All makes sense, will work on that change! Also, it looks like the mobile resources menu is broken again (the menu is not showing) on beta.phlask.me, so will take a look at that as well. but was fixed again here in d71fab9.

So for reference, the commit timeline for the resource menu breaking and getting fixing then should be:

  1. hot fix done in 6b0a4e8
  2. resource menu broke again in 9536e6a
  3. fixed again in this PR (but not merged into develop yet) in d71fab9 while merging develop into the PR branch

ravicodelabs and others added 7 commits August 1, 2024 13:20
@ravicodelabs
Close choose resource modal on mobile after click
e6510fc 
On mobile, we close the choose resource modal upon resource selection to
conserve screen space.
@ravicodelabs
Prefer using createAction
5cdf0dd
@ravicodelabs
Merge branch 'develop' into Ravi/issue-422-refactor-choose-resource-v2
0712638
@ravicodelabs
Change component name to ChooseResourceType
eb48588
@ravicodelabs
Share common data, tweak approach
252167f 
Share common data between Mobile and Desktop ChooseResourceType
components. And, use a more consistent approach and naming for Desktop
and Mobile.
@ravicodelabs
Prefer sx over style prop for mui components
2b151fa
@ravicodelabs
Use strict equality comparison
3134a0f
@ravicodelabs
Copy link
Contributor Author

cc: @gcardonag

The requested update for mobile behavior (to leave the menu open) should be done now. Also, made a few other changes:

  • Renamed the components to use "Type" as in ChooseResourceType, so that it is not confused with the other ChooseResource component in AddResourceModal/ChooseResource.jsx
  • The data for resource types (icons, text label, etc.) is now being shared among both Desktop and Mobile components. There might be room to do so for AddResourceModal as well, but I think that may be out of scope here.
  • Brought up to date with branch develop. However, the build/tests are not working currently as I think we still have to resolve Hotfix for broken selector import #506.

gcardonag
gcardonag approved these changes Aug 7, 2024
View reviewed changes
Copy link
Contributor

@gcardonag gcardonag left a comment

Choose a reason for hiding this comment

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

Thanks for making all of those changes! Approved ✅

@gcardonag gcardonag merged commit 7ff0aaf into develop Aug 7, 2024
6 checks passed
@ravicodelabs ravicodelabs deleted the Ravi/issue-422-refactor-choose-resource-v2 branch August 7, 2024 15:40
@ravicodelabs ravicodelabs self-assigned this Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gcardonag gcardonag gcardonag approved these changes

Assignees

@ravicodelabs ravicodelabs

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@ravicodelabs @gcardonag