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

All view port to mobile -- response #197

Draft
wants to merge 6 commits into
base: all-view-port-to-mobile
Choose a base branch
from

Conversation

lorenjohnson
Copy link
Member

@lorenjohnson lorenjohnson commented Feb 3, 2025

This is a refactor from what you have in your branch @thomasgwatson. Some simple tidy-up and fixes, but also several larger moves here which I do care a fair amount about us making or at least genuinely considering as I do think we need things better factored here to avoid some traps coming in the next steps. I feel much less picky about what happens as we move up the layers from there. . Give it a look and see if you follow. I expect there is a thing or two here not quite right or lost in translation.

What's happening here in this PR branch:

  • Moves ContextWidgetPresenter into @hylo/presenters and, for now, collapsed ViewHelpers and ContextMenuHelpers into that file, propagating that change where it is imported both in Web and Mobile. There is more here to do, like as you suggested, what was in ViewHelpers and ContextMenuHelpers likely is moving more towards GroupPresenter or another new place. I am comfortable with them in this home for now while we sort that out, and until I move GroupPresenter into the presenters module (will do that probably today)
  • Resolves iconName, avatarUrl, and displayName into the ContextWidgetPresenter and then uses these static values in the two WidgetIcon components which does greatly simplify them (nearly making them unnecessary)
  • ContextWidgetPresenter adds a _presented flag to an already presented widget object and checks for that on run of ContextWidgetPresenter(unknownWidget) so we never try and double transform a widget. This allows us FOR NOW to still be lazy about deciding when and if we present a widget and don't need to worry about over doing it if we're not sure it was already presented.
  • For testing in Mobile All Views can now be accessed through the GroupNavigation menu as the last item of a Group
  • Adds new shared CustomViewPresenter for later elaboration as needed, currently only holds the static COMMON_VIEWS object
  • Refactors to use presented widget attribute instead of exporting the resolvers where found
  • Keeps context and null safety by using widget?.attribute vs const { attribute } = widget in a few places
  • Some other things ?

And there are still 3-4 or things I'd like to iron out here, but if we can meet in this version then I can deal with those things... Please let's talk if I'm losing you here. I would still really like to be pair on these pieces until we're both feeling settled, and then dive into the remaining react navigation stuff to get us fully into the context menu setup until a point you can set sail again into that work?

@lorenjohnson lorenjohnson changed the title chore: Consolidates WidgetPresenter logic, makes it re-presentable, a… Consolidates WidgetPresenter logic, makes it re-presentable, and fixes some things Feb 3, 2025
@lorenjohnson lorenjohnson force-pushed the all-view-port-to-mobile-lej branch from 07964f6 to ad37e69 Compare February 3, 2025 11:15
@lorenjohnson lorenjohnson changed the title Consolidates WidgetPresenter logic, makes it re-presentable, and fixes some things All view port to mobile -- response Feb 3, 2025
import mixpanel from 'services/mixpanel'
import { useTranslation } from 'react-i18next'
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, this was just a "while I'm in here" tidy-up. Of no relevance to this PR otherwise.

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.

1 participant