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

[Shape] Text Field and Select's outline-shape-radius functions don't support separate corner values #4140

Closed
kfranqueiro opened this issue Dec 3, 2018 · 5 comments

Comments

@kfranqueiro
Copy link
Contributor

(This is true in 0.41.x and will still be true after #4035.)

The mdc-text-field-outline-shape-radius and mdc-select-outline-shape-radius mixins perform checks which assume that the given radius is a single value. However, most *-shape-radius mixins support multiple values to assign different sizes to different corners.

If you try to pass multiple radius values to one of these mixins, you'll get an error. For example, passing 8px 4px:

On 0.41.x: Error: 12px-8px 4px-1.2 is not a number.

On the #4035 branch: Error: Undefined operation "8px 4px > 12px".

We should investigate whether we can support separate corner values in these mixins, or in the worst case, document that they only support one.

@kfranqueiro
Copy link
Contributor Author

I have WIP for this on fix/outlined-shape-radius (which should probably be split into 2 feature branches to PR, actually). I'm waiting on the outcome of #4171 to continue, though, as it might affect this, and I also realized we probably need to account for ensuring width on the trailing side as well (currently the code in master only accounts for this on the leading side).

@kfranqueiro kfranqueiro modified the milestones: v0.43.0, v0.44.0 Jan 14, 2019
@kfranqueiro kfranqueiro removed this from the v0.44.0 milestone Feb 1, 2019
@kfranqueiro
Copy link
Contributor Author

One of our design advocates has pointed out that it's rather disruptive to encounter this when setting values for entire shape categories at once, so in the nearer term, we should probably handle this with a warning and a graceful fallback to applying just one of the radius values.

Meanwhile, I've moved my branch to feat/textfield-select-shape-radius, updated it against master, added more tests, and fixed some issues I found. Still need to fix an issue of interoperability with trailing icon styles.

@kfranqueiro
Copy link
Contributor Author

Unfortunately, my shorter-term fix in #4547 has now been further set back by #4548, which causes another error due to a value being expanded earlier, resulting in an unexpected nested list. This has, however, caused me to step back and seek to confirm whether assigning multiple radius values to a single shape category is intended/valid. If so, there are probably some things our shape mixins/functions are currently doing (or, more particularly, the order they're being done in) that should be amended.

@kfranqueiro
Copy link
Contributor Author

I just updated the feat/textfield-select-shape-radius branch and resolved any conflicts or oversights from merging with master after #4547.

@kfranqueiro kfranqueiro removed their assignment Apr 5, 2019
@abhiomkar abhiomkar changed the title Text Field and Select's outline-shape-radius functions don't support separate corner values [Shape] Text Field and Select's outline-shape-radius functions don't support separate corner values Apr 15, 2019
@abhiomkar abhiomkar added this-Q and removed backlog labels Apr 19, 2019
@asyncLiz asyncLiz self-assigned this May 21, 2020
@asyncLiz
Copy link
Contributor

This has been fixed in #6163. Outlined textfield/select components now support setting individual corner radii values. Note that filled variants still only support top-left and top-right corners per spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants