-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 View #31542
components: Promote View #31542
Conversation
What would be the reason to keep it internal? We don't have to expose it, but we would have to ensure that documentation isn't published to the block editor handbook. |
Size Change: +19 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
I'm not sure it's really a concept that useful outside of the library building itself. What use does a consumer of |
71128c9
to
a1e0add
Compare
In React Native everything is built from primitive components like View or Text: It all depends if or what do you want to promote instead of using HTML as components. |
The main advantage I see to everything being a View within the library is that we could apply some basic shared styles to it, but is that something that libraries external to |
The codebase is shared between web and native mobile apps so in some places it'd be convenient to be able to use a cross-platform component. In fact, there is already another https://github.com/WordPress/gutenberg/tree/trunk/packages/primitives/src/view |
Should that |
@youknowriad – what should we do with If necessary then in WordPress core, it should be possible to alias |
It sounds a bit weird to me to have emotion in the primitives packages that is only meant to build cross-devices alternatives for the base elements. in other words, why would the primitive be an emotion component? I can understand the hesitation though if at some point the same emotion props could be supported for native too. Maybe for now it's fine to have an internal "View" component (not exposed) in components. |
I'd personally rather not expose this component 👍 If we add styles to |
docs/manifest.json
Outdated
@@ -1175,6 +1175,12 @@ | |||
"markdown_source": "../packages/components/src/v-stack/README.md", | |||
"parent": "components" | |||
}, | |||
{ |
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.
We might have to remove this somehow from the docs.
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 can be listed here:
gutenberg/docs/tool/manifest.js
Line 11 in f444058
ignore: [ '**/src/mobile/**/README.md', '**/src/ui/**/README.md' ], |
This approach doesn't scale well though 😅
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.
Is there a way to do that already or should I add it to the tracking issue as one of the misc tasks to take care of?
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 adding the path to that "ignore" array should be enough for now.
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.
LGTM
8078516
to
457b18a
Compare
Description
Promotes the View component.
Question: should this actually be exported?
How has this been tested?
Storybook still works as expected, unit tests pass and build is successful.
Types of changes
New feature
Checklist:
*.native.js
files for terms that need renaming or removal).