-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
refactor: fix name and state inconsistency in <ThemedLayoutV2>
.
#4404
Conversation
🦋 Changeset detectedLatest commit: d4699ed The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for lighthearted-pastelito-e86ad1 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for refine-doc-live-previews ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for aquamarine-panda-e208bf ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for imaginative-sundae-1fd394 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for stupendous-taffy-33f80c ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
||
export type UseSiderStateType = IThemedLayoutContext; | ||
|
||
export const useSiderState = (): UseSiderStateType => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can change useSiderState
name to useSider
or useThemedSider
.
with that name convention, we can add more features to this hook in the future.
@aliemir @BatuhanW @yildirayunlu what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useSiderState
has more predictable than useSider
or useThemedSider
. I mean, the users can better know what the useSiderState
hook does.
If we want to change the name to add something in the future we can do something more generic like useLayoutContext
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @salihozdemir, useSiderState
is more descriptive than useSider
or useThemedSider
which looks more like useMenu
or useBreadcrumb
and users might confuse them with useMenu
.
We can call it useThemedLayoutContext
, which matches with the name of the context as well (ThemedLayoutContext
).
General
|
…tor/themedlayoutv2
…hemedLayoutContext`
useSiderVisible
is deprecated, instead we created a new hookuseSiderState
for it.useSiderState
similar touseSiderVisible
but it returns more meaningful state names. However,useSiderVisible
is still available for backward compatibility.Updated
Sider
andHamburgerMenu
components usinguseSiderState
.Test plan (required)
Tests not affected.
Self Check before Merge
Please check all items below before review.