[OutlinedInput] Fix offscreen label strikethrough #17680
Conversation
eps1lon
added
bug 🐛
|
@material-ui/core: parsed: +0.19% , gzip: +0.09% Details of bundle changes.Comparing: f1080b6...3aa7f32
|
|
I find the idea interesting, relying on CSS purely sounds better. Sacrificing a bit on the animation probably worth it. In our case, maybe we can accelerate the close animation? Does it still work with a long label? |
I'll have to check what the spec says anyway. I didn't think about accelerating the animation.
Seems like (check "Multiline Placeholder"). Though there's currently an issue with the height (it changes between shrunk states for non dense inputs). |
|
Hey - I would be willing to help you on this since this is also important for us. What can I do? |
|
@eps1lon The new version no longer animates the opening from the center. Was that intentional, or yet to be addressed with the new approach? |
|
Yep, I tried to explain that in the pr description |
oliviertassinari
force-pushed
the
feat/OutlinedInput/notch-html-css-visibility
branch
from
711d276
to
2fbbbc6
Compare
|
@eps1lon I have tried to push this effort one step further. @mbrookes What do you think of this end result? I have tried to reproduce how Google handles the problem in : https://accounts.google.com/signin/v2/identifier. Here is what happens with the latest commit:
I have used the delay and duration to leave a gap at the "right" moment. From my perspective, it's good enough. We should be able to make the logic backward compatible and to solve #18196, #17355, #17305 and #13364. |
|
@oliviertassinari On mobile on the train home, but from what I can tell from the gif, it looks perfect! |
|
@eps1lon What do you think? It seems that we can close 4 issues at once, it has potential :). Do you want to complete the effort with such an animation timing approach? I see the following missing:
|
|
I'd be willing to sacrifice some of the outline animation (the line retracting fro the middle) to have a 100% CSS solution that does not require any JS beyond adding/removing a classname depending on the filled/active state. I'll check this out tomorrow. Solving a common footgun for such an important component would be very nice. |
|
Hopefully this pull request can be revised promptly and merged soon. The workaround referenced in #17355 (which requires removing the content of tabs when switching away) has caused more problems than it solves, due to the fact that I cannot maintain references to components within those tabs.. |
|
@oliviertassinari Could you explain what you tried to achieve with your commit? I can no longer see any transition which is especially confusing since @mbrookes questioned my implementation with some transition but completely agreed with a implementation without any. Seems like I'm missing some context. Also a friendly reminder to not rebase my branches while pushing your commits. The build with my original implementation is now lost. It would've helped to have easy access to how it used to work. |
|
In the current implementation on master, I think that we see the transition too much, meaning, the purpose of it is to allow the user to focus on what he is going to type in the field, and to not distract him with something else. My hope with the commit was that it would enable to preview the change on Netlify, without having the overhead of another pull request. However, it didn't go as expected, instead, we have lost the previous commit preview. Feel free to throw this commit away. I might have solved some conflicts with master too, I don't remember. Sorry, I hoped you wouldn't mind. |
Since the deploy preview failed, I was basing my comment on the gif, which, unless my eyes are deceiving me, does appear to animate. If it doesn't, then the illusion is enough to trick my brain (not that that would be hard!). |
|
@mbrookes Just to clarify: So you were actually opposed to the transition in the first place? My implementation had a transition. Olivier "removed" every transition. "removed" because the smaller max-width is, the more obvious the transition. 150px was small enough to have a transition but obviously only works with small inputs. Olivier used 1000px. At that point the transition is essentially not visible but definitely less visible than before: https://ezgif.com/split/ezgif-6-bb64fd4f2a79.gif |
|
Side note, you are right, the motivation for using 1000px was to support very long labels. I haven't much taken the transition into account in this value. We could likely use a smaller value. Maybe 600px? |
|
The gap can be fixed with the fragment change proposed in #17680 (comment). I think that we can handle it in a follow-up pull request. |
So that was that about. I was confused about mixing |
|
I like codesandbox/ci: https://codesandbox.io/s/material-demo-267kg Still: Please provide a label to your inputs. |
|
Already looking forward to the stress tests we haven't considered 😄 If this PR caused a bug in your application please open a new issue. |
|
btw, shouldn't be |
|
@kusmierz Your comment sounds accurate. Do you want to open a pull request :)? 0.75 is the label scaling factor. We were lucky that the label default font size is 16px and match the default root font-size: 16px. |
|
sure, with pleasure. Please, have a look at #19409 |
|
I'm seeing a slight regression in the label notch as well on chrome: 4.8: 4.9: does this have to do with changing the html font size? will @kusmierz PR fix this? |
|
it should :) |
|
hey guys, 4.9.1 has been released, and now this PR is in |
|
When can we expect a release? |
|
@kelly-tock Already released |
Disclaimer: Not tested cross-browser, yet.
new version
old version
Currently the notch in the outlined input is calculated based on the layout used by the label. This approach does not work if the input is rendered without layout (display: none).
The approach in this PR does not rely on layout computation with JS but purely on css (apart from flipping the notched state). The label is moved inside the legend and always part of the layout (but visibly hidden to avoid visual duplication). Since we can't transition
widthwe transitionmax-width. This comes with the caveat that open transition does not look like it has a fixed length (since we always transition from 0 to a hardcoded value). Closing looks better though.Closes #17355
Closes #17305
Closes #16774
Closes #15713
Closes #16465
Closes #17680
Closes #19213