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

Components with flex items should be aligned on cross axis. #5435

Merged
merged 17 commits into from
Mar 8, 2022

Conversation

alphashr
Copy link
Contributor

@alphashr alphashr commented Dec 3, 2021

📖 Description

Many components use inline-flex for their layout. Couple of these components such as button use start, content and end slot in their template. These slots need to be aligned on the cross axis.

The issue gets more obvious when a control with default look sits next to a same control with an icon or svg within control's start slot or end slot.

🎫 Issues

#5414

👩‍💻 Reviewer Notes

This PR is created to discuss alternative solutions as well. So feel free to comment if you have any objection on the approach.

📑 Test Plan

I am not sure if it breaks any prior usage by users. Maybe they faced this issue and fixed it manually like what has mentioned here? Even in that case this change wouldn't break their custom styling.

✅ Checklist

General

  • I have included a change request file using $ yarn change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

⏭ Next Steps

None.

@alphashr alphashr changed the title Merge pull request flex items should be aligned on cross axis. Dec 3, 2021
@alphashr alphashr changed the title flex items should be aligned on cross axis. components with flex items should be aligned on cross axis. Dec 3, 2021
@alphashr alphashr changed the title components with flex items should be aligned on cross axis. Components with flex items should be aligned on cross axis. Dec 3, 2021
@scomea
Copy link
Collaborator

scomea commented Dec 6, 2021

Perhaps we can bundle this change with the visual refresh that @khamudom is working on to reduce random style changes if we're worried it might interfere with existing implementations?

@alphashr
Copy link
Contributor Author

alphashr commented Dec 8, 2021

Perhaps we can bundle this change with the visual refresh that @khamudom is working on to reduce random style changes if we're worried it might interfere with existing implementations?

Sure, are @khamudom changes going to merge together? and should I take any action?

Copy link
Member

@chrisdholt chrisdholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alphashr!

@chrisdholt
Copy link
Member

Perhaps we can bundle this change with the visual refresh that @khamudom is working on to reduce random style changes if we're worried it might interfere with existing implementations?

Sure, are @khamudom changes going to merge together? and should I take any action?

The visual changes are going to be a major version which means that this fix would only be available once updating - I think it's worthwhile to not wait for that.

@chrisdholt
Copy link
Member

@EisenbergEffect I'm going to merge this later, ideally it fixes a number of problems for folks - it could bit a few that solved this locally, but LGTM for the most part. Let me know if you have hesitations with merging this.

@chrisdholt
Copy link
Member

@alphashr thanks for your patience on this - getting this merged once the build passes :)

@chrisdholt chrisdholt merged commit bb9c277 into microsoft:master Mar 8, 2022
@chrisdholt
Copy link
Member

Thank you @alphashr!

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

Successfully merging this pull request may close these issues.

4 participants