-
Notifications
You must be signed in to change notification settings - Fork 799
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
RNA buttons: add styles and storybook documentation #21496
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
Backup plugin:
|
Just a little comment above, otherwise LGTM! Great job @keoshi! |
Wow, this is awesome, thanks for doing this! |
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.
Wonderful work, Filipe!
I left a few inline comments :)
Out of curiosity, I see you are using the MDX format, I guess because you wish to add custom docs, right? We could take it one step further maybe by utlizing Templates
and Controls
that will allow us, eg, to edit a button's label but I think this is totally optional in this case.
<Story name="Plain button"> | ||
<button type="button" className="components-button is-small">Button</button> | ||
<Story name="Primary button"> | ||
<Button variant="primary" href="#"> |
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 we could omit href
for non-link Buttons.
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 was trying to get us using link Buttons only by not documenting non-link ones. This would make it easier to see the Storybook documentation and grab the right code from there.
Is there a scenario where we currently use a real <button>
element instead of a link as a button? But bottom line, you're right. Maybe these should not be enforced as link-buttons.
|
||
&:focus:not(:disabled) { | ||
color: var( --jp-color-red-50 ); | ||
} |
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.
@import './variables'; | ||
@import './colors'; | ||
|
||
.components-button { |
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 discussed it with the team today, and discovered that these styles will also apply to non-RNA buttons, and in some cases could affect even non-Jetpack elements on the page.
We're discussing possible solutions in Slack: p1635967176436400-slack-CBG1CP4EN
49b3b83
to
31dbd5a
Compare
a { | ||
color: #000; | ||
text-decoration: underline; | ||
} | ||
} | ||
} | ||
|
||
a.jp-connection-screen-icon { | ||
text-decoration: none; | ||
padding-left: 5px; | ||
} |
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.
Reintroducing these lines, were lost in transition when we merged #21521.
They are responsible for styling the "See all Jetpack features" button and the "external" icon:
@@ -86,8 +86,18 @@ | |||
padding-left: 30px; | |||
margin-bottom: 9px; | |||
color: var( --jp-black ); | |||
|
|||
a { |
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.
@sergeymitr Do we need to repeat the link styles in li
? They are also defined in line 60
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 seems like we do. I tried without it, and there was no underline.
Apparently some other style overwrites it.
I didn't dig deep into it 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.
Ok I will look into this more while working on the base styles
Hi @keoshi , While testing this I notices that we don't have buttons styles for Should we add those as well? |
Yeah, we totally should @fgiannar ! I was trying to test those but couldn't get them to work. |
@keoshi I'm thinking to use this PR as the basis for the work needed to properly configure the base styles package. |
Sorry @fgiannar — I missed the latest notification on this PR somehow. The only thing I believe is missing are the active states. Do you need those to be specified beyond what we currently have in Figma? Thank you, appreciate you taking this through the finish line!! |
Hi @keoshi. |
@sergeymitr Is the base-styles package still being discussed, or has it reached a more stable status? |
Howdy y'all — what is the status of this one? I'm going to mark it as "needs author reply" to clean up the "Needs Review" board since there are multiple conflicts and needs a master-merge. |
Closing in favor of #23584 |
Changes proposed in this Pull Request:
@wordpress/components
buttons.Jetpack product discussion
p6TEKc-5rF-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions for Storybook:
cd projects/js-packages/storybook
pnpm install
pnpm run storybook:dev
Screenshot