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

[HUM-132]: chore: separate homepage state hook from context #3005

Conversation

mpblocky
Copy link
Contributor

@mpblocky mpblocky commented Jan 13, 2025

Issue tracking

HUM-132

Context behind the change

  • Separate hook from context

How has this been tested?

  • checked if home page state is correctly provided (e.g. "how it works" in navbar) and functions depending on it are behaving same as before

Release plan

normal deploy

Potential risks; What to monitor; Rollback plan

N/A

Copy link

vercel bot commented Jan 13, 2025

@mpblocky is attempting to deploy a commit to the HUMAN Protocol Team on Vercel.

A member of the Team first needs to authorize it.

@mpblocky mpblocky marked this pull request as ready for review January 24, 2025 14:11
@mpblocky
Copy link
Contributor Author

@dnechay PR ready!

@dnechay dnechay self-requested a review January 24, 2025 14:36
Copy link
Contributor

@dnechay dnechay left a comment

Choose a reason for hiding this comment

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

Nice optimization of context with useMemo

Do we really need to move useHomePageState to shared/hooks? IMO it will only clutter shared/hooks folder.

I would suggest to have shared/contexts/homepage-state folder with internal files like:

  • homepage-state.ts
  • types.ts (seems to be excessive for now, HomePageStateType can be places in context file itself because not used anywhere else so there is no need to even export it)
  • use-homepage-state.ts - hook
  • index.ts - from where you re-export hook and context

wdyt?

@mpblocky
Copy link
Contributor Author

@dnechay I'm not sure about keeping hook in there; do You want to keep context hooks near the context and some more general hooks in shared/hooks?

@dnechay
Copy link
Contributor

dnechay commented Jan 28, 2025

@mpblocky

do You want to keep context hooks near the context and some more general hooks in shared/hooks?

Yes: IMO it's cleaner for our use case, because we don't have many different contexts that we use at the same time in different components and if we co-locate these hooks near to their contexts - it's easier to find things.

In case you anticipate there might be some complex use-cases - then I would keep them in shared/hooks/context

I believe it's better to keep things simple and straightforward till the moment you actually see there is a complex use-case and you need to refactor for it

As an example of a "complex" use case: you might want to have some useColorModeContext hook, that will:

  • attempt to get some shared context first
  • if the above is not available - try to check browser settings
  • if the above is "default" - provide some custom default value
    In this "complex" case you don't just use the same context you defined, you have some extra logic. In this case it's good to place this complex useColorModeContext into some shared/hooks/context folder. But in case you just "get or throw" - it's better to keep it simple and in the same place

@mpblocky
Copy link
Contributor Author

@dnechay yes, this sounds good, just wanted to be sure we are on the same page :)

@mpblocky mpblocky requested a review from dnechay January 28, 2025 13:04
@mpblocky
Copy link
Contributor Author

@dnechay fixed :)

Copy link

vercel bot commented Jan 28, 2025

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

Name Status Preview Comments Updated (UTC)
human-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 28, 2025 3:55pm

@dnechay dnechay merged commit 096b350 into humanprotocol:develop Jan 28, 2025
5 of 9 checks passed
@dnechay dnechay mentioned this pull request Jan 28, 2025
35 tasks
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.

3 participants