-
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
Style engine: refine Box
type
#38894
Conversation
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, this is clever, thanks for the follow-up @ciampo! The other use case that we will have for the Box
type that isn't covered yet is when we add in position support, where the lower case top
, right
, bottom
, and left
will be used directly. Is there a way to have those words be lowercase if BoxVariants
is an empty string?
(For reference, here's an early WIP PR looking at adding in position support — it'll likely change a lot, but just for context as to why it was on my mind 😀)
You raise a good point! I went ahead and allowed I quickly tested it in my IDE and it seems to work as expected (note that box-variants-ts.mp4 |
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.
Nice one, thanks @ciampo — that's looking good to me now, and confirmed that it's working the same in my IDE as it is in your screengrab 👍
Looks like there's a failing e2e unrelated to this PR, so it might need either a rebase or to kick off the tests again before you merge it in.
Thanks again for the follow-up!
7dd7818
to
fa85133
Compare
Description
Follow-up to this conversation, this PR proposes a more advanced type for
Box
which is closer to the actual values thatBox
is going to assume.Testing Instructions
Screenshots
Types of changes
Enhancement
Checklist:
*.native.js
files for terms that need renaming or removal).