-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Components: promote Scrollable
#32446
Conversation
Size Change: +702 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
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.
Looking good, testing well 👍
@@ -44,7 +44,8 @@ | |||
"src/h-stack/**/*", | |||
"src/v-stack/**/*", | |||
"src/z-stack/**/*", | |||
"src/view/**/*" | |||
"src/view/**/*", | |||
"src/scrollable/**/*" |
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.
Nit: should these be sorted alphabetically?
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.
I initially added this line in alphabetical order, but then noticed that the list is sorted until src/visually-hidden/**/*
— and then I noticed that the "sorted" part of the list represents components that are marked as production-read, while the second (non-sorted) part is for experimental components.
Probably the best person to answer about this would be @sarayourfriend , but maybe we keep this change as-is for now and if needed sort this list alphabetically in one of the future PRs?
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.
Good point! I tried to give an explanation here
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.
Oh, that's a good point 👍 Thanks for elaborating
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.
Hmmm, I don't think any of that is intentional. Sorting them alphabetically is fine with me. If we want to make a separation between the experimental and prod components we should add a comment explaining that and add some spacing to make it more obvious.
Though TBH I don't think it's worth the effort to maintain two separate lists. I'd rather we just opened a PR like Marco suggested to sort them once and for all.
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.
Just wanted to stress that this is totally a minor deal. Maybe we can sort these when we touch the list the next time, but there's definitely no need to make a special PR just to sort them. It's why I prefaced my first comment with "nit: " 😉
|
||
export default { | ||
component: Scrollable, | ||
title: 'G2 Components (Experimental)/Scrollable', | ||
title: 'Components (Experimental)/Scrollable', |
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.
I think it would have been useful if the story allowed us to change the scroll direction and toggle the smooth scroll option.
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.
Done in dcaafa7
import { useScrollable } from './hook'; | ||
|
||
/** | ||
* `Scrollable` is a layout component that content in a scrollable container. | ||
* | ||
* @example | ||
* ```jsx | ||
* import { Scrollable } from `@wordpress/components/ui`; | ||
* import { __experimentalScrollable as Scrollable } from `@wordpress/components/ui`; |
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.
docs/manifest.json
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.
Hey @nenel1221 , I'm not sure I'm getting your comment — could you elaborate, please?
Description
Promote the
Scrollable
component from thepackages/components/src/ui
to thepackages/components/src
folder.@wordpress/components
as__experimentalScrollable
Testing instructions
trunk
)Screenshots
Types of changes
Refactor
Checklist:
*.native.js
files for terms that need renaming or removal).