fix(components): Change label FomField label to have full width

[nad182]

Motivations

The bug with label not taking full text area width was originally reported here.

Changes

Changed

• CSS of textarea:
  • Gave label the width of 100%
  • Changed the paddings of input to accommodate label
  • This causes the scroll bar to appear differently: it starts the top of the input (underneath the label), instead of from the very top of the FormField (which included the label).

Testing

WAS

IS

Changes can be tested via Pre-release

---

In Atlantis we use Github's built in pull request reviews.

[ZakaryH]

if we are going the full width with no padding, there's one scenario where things will look different

• placeholder is unreasonably long
• the userAgent is not reported back as Safari and so we don't get the extra margin

now our placeholder content goes to the very edge of the input rather than having some spacing on the right. I do think this is very unlikely to happen.

whereas Safari reporting userAgents will handle this fine due to the extra margin right

[ZakaryH]

ah another thing to consider is that with a padding on the top, we won't always have a label. so without a label, and that padding top we go from

to

where the text that scrolls upwards now disappears into an empty white space compared to it feeling like it went into/above the textarea

also the scrollbar doesn't go the full height. that one is pretty minor IMO. I don't know how many people would notice that, plus there's some other strange behavior with scrollbars in this code to begin with

[nad182]

ah another thing to consider is that with a padding on the top, we won't always have a label. so without a label, and that padding top we go from

to

where the text that scrolls upwards now disappears into an empty white space compared to it feeling like it went into/above the textarea

also the scrollbar doesn't go the full height. that one is pretty minor IMO. I don't know how many people would notice that, plus there's some other strange behavior with scrollbars in this code to begin with

That one should be relatively straightforward to fix (I think). I can just make it so that the padding is added ONLY if there is a label. That should fix the scrollbar starting from the top as well.

[ZakaryH]

for context I remember what all the regex/Safari stuff was about now

when you use an iphone/apple device in Chrome's devtools for a mobile view - it replicates the userAgent that would be reported. on apple devices, it gets identified (correctly) as Safari since I believe that's the underlying engine or whatever that influences how the browser will behave

that's how you get these letters appearing in a bad place

that's because it's using margin and not padding in this scenario, which is intentional to leave room to interact with the draggable handle on Safari. other browsers handle the zIndex in a nice way where you can still interact with the handle despite it technically being "behind" another element. in order for Safari to access it, we've gotta add that margin.

but since it's margin, we're basically already accounting for the space it takes up. then the width - padding value is applied, which isn't needed in this specific case and it removes padding again, leaving that space for characters to show up

your fix will address this, we'd just need to sort out those couple extra scenarios. alternatively, if we wanted to try a different approach we could make that width - padding rule conditional, and not apply it when we're on a Safari instance. that might end up being easier? I haven't fully thought through that case, but based on my understanding I could see that being all that's required.

[cloudflare-workers-and-pages[bot]]

Deploying atlantis with    Cloudflare Pages

Latest commit:	4e454bf
Status:	✅  Deploy successful!
Preview URL:	https://22439426.atlantis.pages.dev
Branch Preview URL:	https://job-99899.atlantis.pages.dev

View logs

[nad182]

@ZakaryH , I pushed the fix to add the padding on the right, since the label is occupying the full width with my change in the previous commit. This should fix the case when the label is overly long. I've also added some logic to add and remove top paddings to the input (and the wrapper) based on whether the label is provided (present) or not. Please, let me know what you think.

PS. This also fixes the scrollbar to start from the top in case there is no label.

[ZakaryH]

ah, we're starting to get into the fun of FormField

the "basic" or non multiline variant of InputText doesn't work so well with the childWrapper padding. same thing for pretty much any other FormField relative (InputDate, InputTime, InputNumber etc.)

vs master

it's because the input has its own padding that handles things AND it has a label, so now it's double dipping on padding 😭

you're welcome to keep going with this approach but I do want to mention another possible approach

your fix will address this, we'd just need to sort out those couple extra scenarios. alternatively, if we wanted to try a different approach we could make that width - padding rule conditional, and not apply it when we're on a Safari instance. that might end up being easier? I haven't fully thought through that case, but based on my understanding I could see that being all that's required.

it wouldn't address the scrollbar height stuff, but I think this would solve the initial bug in a way that involves less changes . I'm pretty sure anyway, admittedly I haven't tried it out!

[nad182]

ah, we're starting to get into the fun of FormField

the "basic" or non multiline variant of InputText doesn't work so well with the childWrapper padding

vs master

it's because the input (not textarea) has its own padding that handles things AND it has a label, so now it's double dipping on padding 😭

you're welcome to keep going with this approach but I do want to mention another possible approach

your fix will address this, we'd just need to sort out those couple extra scenarios. alternatively, if we wanted to try a different approach we could make that width - padding rule conditional, and not apply it when we're on a Safari instance. that might end up being easier? I haven't fully thought through that case, but based on my understanding I could see that being all that's required.

it wouldn't address the scrollbar height stuff, but I think this would solve the initial bug in a way that involves less changes . I'm pretty sure anyway, admittedly I haven't tried it out!

@ZakaryH , I've tried adding :not(.safari) selector and it works (in Safar), but it doesn't work in other browsers in mobile view (Samsung selected in the inspector):

Also, just like you said, the scrollbar is being hidden behind the label, which was the bug @MichaelParadis fixed 14 moths ago. So I'm worried that a simple change for label to have a width: 100% and padding-right: 0, will just reintroduce that bug.

[ZakaryH]

ah I think the difference is that I'm trying to do this on a normal Safari browser

the extra space to not cut off the scrollbar isn't quite enough here since the space-small is being applied as margin and not counted as part of the container that hold the scrollbar. we'd need a bit more to properly leave room for the scrollbar

but also on Chrome with the dev tools simulating an iphone, we have the initial problem of letters being visible in the space

I'm not sure if there's gonna be a great/reliable way to leave the room for the scrollbar. I think your initial idea of using padding top might be better. I'm gonna go back to one of those commits and see if we can get it working with a minor tweak. I think you were really close at one point!