15087: Add Button Base #15998
Merged
15087: Add Button Base #15998
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
6 tasks
garrettbear
commented
Sep 27, 2022
| it('should render with different button states', () => { | ||
| const { getByTestId } = render( | ||
| <> | ||
| {/* <ButtonBase loading data-testid="loading" /> */} |
There was a problem hiding this comment.
Commented this one out because the loading icon is using Icon component, but in testing doesn't have access to the ICON_NAMES
There was a problem hiding this comment.
not sure that the MOCK_ICON_NAMES will actually solve this either since it is referring to the loading icon which is nested inside the component.
There was a problem hiding this comment.
One work around is to add loadingIconProps to the button component and pass in the name for this test just to make sure it's working for tests.
There was a problem hiding this comment.
Does this still need addressing?
There was a problem hiding this comment.
Do we think loadingIconProps is worth adding to the button just for the test?
Is there something we can do to the Icon component so we can avoid doing workarounds on all of our future components that use the icon?
There was a problem hiding this comment.
Yup something that will need to be fixed for Icons. I've created an issue #16046 I think we can move forward with this PR and resolve this test once this issue has been resolved
| it('should render with different button states', () => { | ||
| const { getByTestId } = render( | ||
| <> | ||
| {/* <ButtonBase loading data-testid="loading" /> */} |
ui/components/component-library/button-base/button-base.test.js
Outdated
Show resolved
Hide resolved
Comment on lines
+85
to
+92
| &--inherit { | ||
| font-family: inherit; | ||
| font-weight: inherit; | ||
| font-size: inherit; | ||
| line-height: inherit; | ||
| letter-spacing: inherit; | ||
| } | ||
|
|
There was a problem hiding this comment.
Should any documentation in the Text component reflect this update?
There was a problem hiding this comment.
Yea probably should. I will add a quick something
Builds ready [4d13701]
Page Load Metrics (2223 ± 55 ms)
highlights:storybook
|
Builds ready [87cd0ed]
Page Load Metrics (2443 ± 91 ms)
highlights:storybook
|
There was a problem hiding this comment.
Looking really good! I was playing around with making the default display: inline-flex; so when the root element is changed to an a tag it doesn't break but no biggy. Cause I know it will generally be used with ButtonLink type and then we also would need have some irrelevant flex options so up to you 🤷
ui/components/component-library/button-base/button-base.test.js
Outdated
Show resolved
Hide resolved
add button constants button base updates base button docs update add iconprops base button update block doc on button
garrettbear
force-pushed
the
feat/15087/button-base-component
branch
from
526f91c to
c903ed4
Compare
garrettbear
commented
Sep 29, 2022
garrettbear
added
team-design-system
Builds ready [54f05f6]
Page Load Metrics (2257 ± 123 ms)
highlights:storybook
|
georgewrmarshall
previously approved these changes
Sep 29, 2022
darkwing
previously requested changes
Sep 30, 2022
| 'mm-button', | ||
| `mm-button--size-${size}`, | ||
| { | ||
| [`mm-button--loading`]: Boolean(loading), |
There was a problem hiding this comment.
I don't believe we need the wrapping [] around the classname keys here and the next few lines.
There was a problem hiding this comment.
Correct, no need for the [] 👍
| it('should render with different button states', () => { | ||
| const { getByTestId } = render( | ||
| <> | ||
| {/* <ButtonBase loading data-testid="loading" /> */} |
There was a problem hiding this comment.
Does this still need addressing?
segun
reviewed
Sep 30, 2022
| border-radius: 999px; | ||
| cursor: pointer; | ||
| color: var(--color-text-default); | ||
| background-color: #dcdcdc; |
There was a problem hiding this comment.
should we be using a variable here as well?
There was a problem hiding this comment.
Probably, all ask the designer if we want to use a different color. #dcdcdc color is used in the design, but we currently don't have that as a defined token(variable) color
There was a problem hiding this comment.
Designer said it's fine to leave as is, but can also use Brand/Grey100 (#D6D9DC). I will update using that variable.
There was a problem hiding this comment.
Nice catch @segun! We shouldn't have any static hex values in our code base if we can avoid it :)
georgewrmarshall
approved these changes
Sep 30, 2022
Builds ready [ce4d401]
Page Load Metrics (2468 ± 107 ms)
highlights:storybook
|
georgewrmarshall
previously approved these changes
Sep 30, 2022
georgewrmarshall
previously approved these changes
Oct 3, 2022
Co-authored-by: George Marshall <[email protected]>
georgewrmarshall
previously approved these changes
Oct 3, 2022
georgewrmarshall
approved these changes
Oct 3, 2022
Builds ready [25bf9c4]
Page Load Metrics (2174 ± 91 ms)
highlights:storybook
|
Builds ready [1021345]
Page Load Metrics (2377 ± 64 ms)
highlights:storybook
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Adding new Design System Button:
The ButtonBase can be used when needing to build a customized button, which can then be proposed to the design system.
[Figma].(https://www.figma.com/file/HKpPKij9V3TpsyMV1TpV7C/DS-Components?node-id=11%3A15)
More Information
Fixes #15087
Screenshots/Screencaps
Manual Testing Steps
yarn test:unit:jest .ui/components/component-library/button-base/button-base.test.jsPre-Merge Checklist
+ If there are functional changes: