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

Clean-up Flex layout align configuration #503

Closed
rajsite opened this issue Apr 18, 2022 · 1 comment · Fixed by #1725
Closed

Clean-up Flex layout align configuration #503

rajsite opened this issue Apr 18, 2022 · 1 comment · Fixed by #1725
Assignees

Comments

@rajsite
Copy link
Member

rajsite commented Apr 18, 2022

🧹 Tech Debt

We had found icons and controls were not aligning property. One workaround was to add the vertical-align: middle property on the button as show here:

See: https://github.com/microsoft/fast/issues/5414

In the FAST issue they found an alternate pattern that just changes flex settings: microsoft/fast#5414 (comment)

We should verify that configuration works and propagate it to our controls where it makes sense. We should also add tests focused on validating inline behavior of the controls (ie inline with text and inline-block images, etc).

@rajsite rajsite added tech debt triage New issue that needs to be reviewed labels Apr 18, 2022
@m-akinc m-akinc added tiny🐜 and removed triage New issue that needs to be reviewed labels Apr 26, 2022
@m-akinc m-akinc self-assigned this Jan 3, 2024
@m-akinc m-akinc moved this from Backlog to In progress in Nimble Design System Priorities Jan 3, 2024
@m-akinc
Copy link
Contributor

m-akinc commented Jan 3, 2024

FAST's approach results in alignment that is inconsistent with native button elements.

FAST's approach (updated stackblitz to latest FAST packages):
image

Nimble's current approach (setting vertical-align: center on host):
image

Native button element:
image

I tried modifying FAST's approach by setting the inner, native button's align-items to something other than baseline, but that results in misaligned buttons (i.e. the original problem):
image

My conclusion is that we should keep our current approach. I will still create a new Chromatic test for button alignment, though.

m-akinc added a commit that referenced this issue Jan 4, 2024
## 🤨 Rationale

Fixes: #503 

## 👩‍💻 Implementation

Added a button Chromatic test for how buttons align with inline text and
images.

## 🧪 Testing

New test added.

I noticed that the Safari snapshot of the new Chromatic test has
unexpected alignment (top of text is aligned with top of buttons). From
what I can tell, Chromatic is using version 16 of Safari. I checked the
new story in versions 15.x and 17.1, and both rendered with the
alignment I would expect (same as other browsers). I also tried it in
Playwright's WebKit browser version 16.0, and I still got the expected
alignment. I do not know how Chromatic ended up with weird alignment,
but I'm satisfied enough that it looks right in all versions of
Safari/WebKit I checked.

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.

---------

Co-authored-by: Milan Raj <[email protected]>
@m-akinc m-akinc moved this from In progress to Done in Nimble Design System Priorities Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

2 participants