GetJobber / atlantis

🔱 Atlantis
https://atlantis.getjobber.com
MIT License
27 stars 30 forks source link

fix(components): Remove center alignment in FormField #2057

Closed taylorvnoj closed 1 month ago

taylorvnoj commented 1 month ago

Motivations

Reverting this align-items & transform change from this PR https://github.com/GetJobber/atlantis/pull/2052/files

Text in the container was also centering - very obvious on multiline text fields

We want this (multiline InputText) Screenshot 2024-10-03 at 11 55 50 AM

Not this Screenshot 2024-10-03 at 11 56 57 AM

And to make sure clearable looks as it did before: Screenshot 2024-10-03 at 11 57 42 AM

Changes

Added

Changed

Deprecated

Removed

Fixed

Security

Testing

Changes can be tested via Pre-release


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

Random photo of Atlantis

cloudflare-workers-and-pages[bot] commented 1 month ago

Deploying atlantis with  Cloudflare Pages  Cloudflare Pages

Latest commit: 181c079
Status: âœ…  Deploy successful!
Preview URL: https://aa82c327.atlantis.pages.dev
Branch Preview URL: https://taylor-fix-formfield-alignme.atlantis.pages.dev

View logs

github-actions[bot] commented 1 month ago

Published Pre-release for 181c079a2017121e25bd9231d59be9edb34ff811 with versions:

  - @jobber/components@5.40.1-TAYLORfix-181c079.1+181c079a

To install the new version(s) for Web run:

npm install @jobber/components@5.40.1-TAYLORfix-181c079.1+181c079a
ZakaryH commented 1 month ago

I have a question about the problem the initial work was looking to resolve - how/when was that happening?

the clearable button shows up where I'd expect it to when it has no placeholder image

and with a placeholder image

taylorvnoj commented 1 month ago

I have a question about the problem the initial work was looking to resolve - how/when was that happening?

the clearable button shows up where I'd expect it to when it has no placeholder image

and with a placeholder image

On large size inputs: It was like this Screenshot 2024-10-03 at 12 38 14 PM

and we noticed with the initial fix it was better aligned Screenshot 2024-10-03 at 12 38 21 PM

But I didn't test multiline because multiline inputs can't have clearable. BUT I didn't realize the entire context of the text input would also align center

chris-at-jobber commented 1 month ago

Note for future: I think if we want to solve that clearable placement on large inputs, something along the lines of

position: absolute;
top: 50%;
tranform: translateY(-50%);

OR

wrap the clearable in a container, set that container to take up the full height of the input, and use flex to manipulate the position of the clearable button

Might be safer paths we take to get there. I think we have this working as expected in mobile so maybe we can take a peek at what we did there!

ZakaryH commented 1 month ago

@chris-at-jobber I also thought the top: 50% + position: absolute would fix it, which technically it does - but it breaks other combinations

https://github.com/GetJobber/atlantis/pull/1856#issuecomment-2062480661

if it's absolute, then it will overlap with other elements like the loading spinner or icons that can be in the same general area. which is why we made it relative.

we could try the container approach though, as long as that lets sibling elements position correctly. I do wonder what that's going to do to the available space for the actual text input though 🤔