search

phlask / phlask-map

Code behind the Phlask Web Map
https://beta.phlask.me
34 stars 36 forks source link

Consolidate how the resource modal is rendered on mobile and desktop to potentially use the approach built in PR #332 #422

Closed ravicodelabs closed 3 months ago

ravicodelabs commented 6 months ago

Is your feature request related to a problem? Please describe.

There appears to be opportunity to consolidate the approaches for how the resource modal is rendered on mobile and desktop. Currently two different approaches are used, which does not seem ideal from a maintenance or code organization point of view.

In PR #332, the action TOOLBAR_MODAL_RESOURCE was added, and is currently being used for the desktop version. This can be seen in component ChooseResource.js.

On the other hand, the mobile approach currently uses action TOGGLE_RESOURCE_MENU. This can be seen in component ResourceMenu.js. The relevant code for this approach can be seen in PR #421, which might need to be removed after the refactoring.

Describe the solution you'd like

The goal is to assess what the best approach for the resource modal is, and use a unified approach for both desktop and mobile. This might entail integrating component ResourceMenu.js into component ChooseResource.js, however more investigation needs to be done to be sure.

Additional context

Related PRs: #332, #421 Related issues: #401, #345

Mobile resources menu screenshot:

Resources_Menu_Shown

Desktop resources menu screenshot:

Resources_Menu_Shown-Desktop
tomporvaz commented 6 months ago

@ravicodelabs I totally agree with your approach here. Now that I have fully read this new issue that you created, I understand this to be the work that needs to be done after the hotfix for issue #401. I left issue #401 open for the continuation of refactoring you are planning on doing after the hotfix, but now I am realizing the hotfix (PR #421) should close issue #401. Does this sound right? Are you planning on doing any more work on issue #401?

ravicodelabs commented 6 months ago

...the hotfix (PR #421) should close issue #401. Does this sound right? Are you planning on doing any more work on issue #401?

Yep, that sounds right. I would agree that #401 has been addressed with the hotfix and can be closed now. And, I don't plan on doing anymore work on #401. Thanks!

tomporvaz commented 4 months ago

@ravicodelabs I am unable to get this branch running locally, but I wanted to check the design of the resources menu didn't change with this update. My understanding of this ticket was that you were changing the architecture of the code, and as I mentioned, this seems like a great idea. However, we do have designs for the mobile resources menu in figma and we should adhere to that.
image

ravicodelabs commented 4 months ago

Hey @tomporvaz - yes the designs should match what are in the Figma. On the other hand, something appears to have broke on the last sync with the main branch, so that's probably why you weren't able to get it running. I will look into that more closely and report back. Thanks!

ravicodelabs commented 4 months ago

Hey @tomporvaz, there were quite a few changes needed, so it was more straightforward to just create a new PR - #463 - which addresses this issue. Hopefully you should be able to run it as needed now, but let me know if there's any issue.

cc: @gcardonag @vontell

gcardonag commented 4 months ago

Thanks for working on this Ravi! I'll give that PR a review tonight.