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

add support for directional border shorthands #95

Closed
wants to merge 2 commits into from
Closed

Conversation

quantizor
Copy link

No description provided.

@jacobp100
Copy link
Contributor

Thanks for the PR!

I held off this because of the lack of support for setting the border style of just one side. I'd worry with this we'd get bug reports of people saying it set the border style of all sides

@quantizor
Copy link
Author

quantizor commented Oct 13, 2018 via email

@jacobp100
Copy link
Contributor

We could definitely give a better error message when they try to use it

@jacobp100
Copy link
Contributor

My gut feeling is that we don't add it, or just don't allow setting the border style (and warn if they do). But if everybody else is happy with this as-is, I'm happy for it to be merged.

@kristerkari what do you think?

@kristerkari
Copy link
Contributor

Hmm.. this is tricky one because I guess that the idea is to only support properties that we can make work the same way than in CSS.

If I understood correctly supporting this would give a different result if you use the React Native CSS in the browser? If that's the case then I vote against merging it.

@kristerkari
Copy link
Contributor

kristerkari commented Oct 15, 2018

just don't allow setting the border style (and warn if they do).

I think that this is something that we could do instead.

@jacobp100
Copy link
Contributor

Although if your argument was it should match web 100%, omitting the border style is a syntax error, so wouldn’t match web

@kristerkari
Copy link
Contributor

Although if your argument was it should match web 100%, omitting the border style is a syntax error, so wouldn’t match web

Oh yeah I did not think about that. In that case my opinion is that let's not support it if it's not compatible with Web. RN might anyway add support in the future for setting border style for different sides.

kopax pushed a commit to kopax/css-to-react-native that referenced this pull request Nov 30, 2018
@mgcrea
Copy link

mgcrea commented Jan 22, 2019

Would love to have this, having to manually user border-bottom-width, border-bottom-color instead of a shorthand is a pain. Regarding the single-border style issue, I think it's a really minor use case as I think most of the time you really only want a single solid border so it would work.

@kristerkari
Copy link
Contributor

Would love to have this, having to manually user border-bottom-width, border-bottom-color instead of a shorthand is a pain.

I agree that it is painful not being able to use the shorthands, but it is still a much better option than causing unexpected results and not rendering the same way is it does in browsers. Currently you should be able to use the same CSS in React Native and the browser to render the same way.

I hope that in the future React Native would support setting borderStyle for each side, and then we should definitely add the shorthands.

@jacobp100
Copy link
Contributor

So I looked at this in a bit more detail

We could never allow border-right: 2px solid dashed, as this would not work in RN.

But we could allow border-right: 2px red, and throw if they try to define a border style. However, that could never be web compatible, as on RN, the border style would be inherited, and on web, it would default to none, so show no border

I also noticed we allow border: 2px red currently, and this is not compatible on web

(And lastly, I the border shorthand did not output directional properties like margin and padding do)

So I opened up two PRs

I feel like the first option is the more pure version, but it is breaking change. It's also consistent with what we do in other places. If you use a unitless line-height in a font shorthand, you define multiple box-shadows, and percentage widths on border widths

What do other people think?

@kristerkari
Copy link
Contributor

I feel like the first option is the more pure version, but it is breaking change. It's also consistent with what we do in other places. If you use a unitless line-height in a font shorthand, you define multiple box-shadows, and percentage widths on border widths

My opinion is the same as before: to keep Web compatibility. If we make exceptions to that, then we break the universal (Web/RN) rendering of styles. I think that with being a bit strict about the styles we can provide a much better experience for all users.

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.

None yet

4 participants