Skip to content
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

Allow Border color, width and style to be customized via style prop #3965

Conversation

judygab
Copy link
Contributor

@judygab judygab commented May 22, 2023

Description

  1. Added border color, style, and width to Style Props so components that have prop types that extend StyleProps are customizable not only by border but with also attributes like borderColor, borderWidth, borderStyle

Issue #, if available

Fixes: #3945

Description of how you validated changes

Checklist

  • Have read the Pull Request Guidelines
  • PR description included
  • Relevant documentation is changed or added (and PR referenced)
  • yarn test passes and tests are updated/added
  • No side effects or sideEffects field updated

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@judygab judygab requested a review from a team as a code owner May 22, 2023 18:55
@changeset-bot
Copy link

changeset-bot bot commented May 22, 2023

🦋 Changeset detected

Latest commit: befe3e8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@aws-amplify/ui-react Patch
@aws-amplify/ui-react-liveness Patch
@aws-amplify/ui-react-notifications Patch
@aws-amplify/ui-react-storage Patch

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

@hbuchel hbuchel added run-tests Adding this label will trigger tests to run and removed run-tests Adding this label will trigger tests to run labels May 22, 2023
Copy link
Contributor

@hbuchel hbuchel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @judygab! Thank you for submitting this PR! I gave it a test drive and the style props definitely work.

Screenshot of Divider component example code that uses the borderColor, borderStyle, and borderWidth props

One thing that would be great to add is allowing our theme tokens to be usable as values of the borderColor and borderWidth props like how our other style props work. Essentially, we should be able to do <Divider borderColor="red.10" borderWidth="small" /> where red.10 is a color token and small is a borderWidth token.

You can compare how this is done with backgroundColor with the ColorKeys type and the mapping to theme tokens here.

@judygab
Copy link
Contributor Author

judygab commented May 22, 2023

@hbuchel thanks for the feedback! I have just committed changes adding color and space theme tokens to borderColor and borderWidth

Copy link
Contributor

@hbuchel hbuchel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @judygab Looking good! Left one small comment. Additionally, can you please update the test snapshot? That will fix the failing tests. Inside packages/react you can run yarn test -- -u and then commit the changes to the updated snapshot file.

@judygab
Copy link
Contributor Author

judygab commented May 23, 2023

@hbuchel sure! how can I update snapshot tests? When I run yarn test -- -u I do see that all test are failing

@hbuchel
Copy link
Contributor

hbuchel commented May 23, 2023

@hbuchel sure! how can I update snapshot tests? When I run yarn test -- -u I do see that all test are failing

Try running that from within /packages/react

cd packages/react
yarn test -- -u

That should result in the snapshot file updating (which you can commit those changes) and then tests should also pass.

@judygab
Copy link
Contributor Author

judygab commented May 24, 2023

@hbuchel sure! how can I update snapshot tests? When I run yarn test -- -u I do see that all test are failing

Try running that from within /packages/react

cd packages/react
yarn test -- -u

That should result in the snapshot file updating (which you can commit those changes) and then tests should also pass.

that's exactly what I did but it didn't update any files,
Screen Shot 2023-05-23 at 10 13 45 PM
just resulted in:

@hbuchel
Copy link
Contributor

hbuchel commented May 24, 2023

that's exactly what I did but it didn't update any files, Screen Shot 2023-05-23 at 10 13 45 PM just resulted in:

Hmmm that shouuuuuuld have worked. Maybe you need to build the repo first? You can do that with a yarn build at the root. Then try those commands again^

I also pushed a change to the branch on our repo, you should be able to update your PR with it judygab/amplify-ui@react/border-width-color-style-via-style-props...aws-amplify:amplify-ui:react/border-width-color-style-via-style-props

@judygab
Copy link
Contributor Author

judygab commented May 24, 2023

that's exactly what I did but it didn't update any files, Screen Shot 2023-05-23 at 10 13 45 PM just resulted in:

Hmmm that shouuuuuuld have worked. Maybe you need to build the repo first? You can do that with a yarn build at the root. Then try those commands again^

I also pushed a change to the branch on our repo, you should be able to update your PR with it judygab/amplify-ui@react/border-width-color-style-via-style-props...aws-amplify:amplify-ui:react/border-width-color-style-via-style-props

could be, will try that, thanks! Also updating my branch. Anything else for this pr?

@hbuchel
Copy link
Contributor

hbuchel commented May 25, 2023

could be, will try that, thanks! Also updating my branch. Anything else for this pr?

@judygab this change to update the borderWidth tokens to us borderWidths instead of space and updating the test snapshot are the remaining changes. Thanks!

@judygab
Copy link
Contributor Author

judygab commented May 25, 2023

could be, will try that, thanks! Also updating my branch. Anything else for this pr?

@judygab this change to update the borderWidth tokens to us borderWidths instead of space and updating the test snapshot are the remaining changes. Thanks!

@hbuchel please see the updated pr, made those 2 changes.

@hbuchel
Copy link
Contributor

hbuchel commented May 25, 2023

@judygab I realized with some further testing of the props that we needed to export BorderWidthKeys from the ui package to use with our borderWidth type. Tested with the Divider examples; the first screenshot shows the type autocompletes showing all of the space keys. The second screenshot shows the correct borderWidth keys after this change.

Autocomplete dropdown in VSCode for borderWidth prop showing all of our space design tokens Autocomplete dropdown in VSCode for borderWidth prop showing all of our borderWidth design tokens

I made that change in this commit here I also merged in changes from main; please sync from this branch and update your PR. Thanks! After that I'll ping the team for further reviews.

@judygab
Copy link
Contributor Author

judygab commented May 31, 2023

@hbuchel oh I see so it was just the type export that we were missing, right?
Did sync and update my branch, let me know if there's anything else.

Copy link
Contributor

@hbuchel hbuchel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @judygab! Smoke tested the last changes and it looks good to me.

Example Card with outlined variation using borderStyle, borderWidth, and borderColor style props Example Divider using borderStyle, borderWidth, and borderColor style props

I'll ping the team about getting this included in our next release.

@hbuchel
Copy link
Contributor

hbuchel commented Jun 6, 2023

@judygab Two updates I pushed here if you can please merge those into your PR:

  • Updated the docs snapshot which will fix the failing test. We auto-generate our props documentation so the snapshot needed to be updated in the docs as well. I included that in the commit I linked, but just in case for future reference, you can run this command from the docs folder:
    yarn test -- -u
  • Updated our changeset to include the ui package

@hbuchel
Copy link
Contributor

hbuchel commented Jun 23, 2023

Hi @judygab just an FYI we merged these changes as part of a separate PR, just to make merging all the changes from main a little easier since there have been some major releases recently. Thank you so much for this feature contribution!

@hbuchel hbuchel closed this Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR (Primitives): Allow Divider color, width and border style to be customized via style prop
4 participants