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

mis-aligned slots in fast-menu-item (and fast-button) #3904

Closed
ben-girardet opened this issue Sep 15, 2020 · 10 comments · Fixed by #4081
Closed

mis-aligned slots in fast-menu-item (and fast-button) #3904

ben-girardet opened this issue Sep 15, 2020 · 10 comments · Fixed by #4081
Assignees
Labels
bug A bug closed:done Work is finished community:good-first-issue Good issues for first time contributors community:request Issues specifically reported by a member of the community.

Comments

@ben-girardet
Copy link
Contributor

ben-girardet commented Sep 15, 2020

Describe the bug; what happened?

When using the fast-menu-item component and placing content in the start slot it fails to be nicely align (vertically) with the menu item text.

EDIT: This mis-alignment in fact also happen in <fast-button> and probably comes from something not totally figured out, see my comment below.

What are the steps to reproduce the issue?

  1. Open the fast explorer in the fast-menu section
  2. Add a fast-button in the start slot
  3. The result should look like this:

Capture d’écran 2020-09-15 à 22 19 47

What behavior did you expect?

A nice vertical alignement between each part of the fast-menu-item template.

@ben-girardet ben-girardet added the status:triage New Issue - needs triage label Sep 15, 2020
@chrisdholt
Copy link
Member

Thanks @bengirardet! Let us know if you'd like to submit a fix for this. If not, it should be fairly straight forward for us to fix.

@chrisdholt chrisdholt added area:fast-components bug A bug community:good-first-issue Good issues for first time contributors community:request Issues specifically reported by a member of the community. status:needs-investigation Needs additional investigation and removed status:triage New Issue - needs triage labels Sep 16, 2020
@chrisdholt chrisdholt added this to the Release 12 milestone Sep 16, 2020
@ben-girardet ben-girardet changed the title mis-aligned slots in fast-menu-item mis-aligned slots in fast-menu-item (and fast-button) Sep 16, 2020
@ben-girardet
Copy link
Contributor Author

I've edited the title and description in order to reflect that this issue affects several components, at least menu-item and button.

Seems like there are still things that need to be figured out regarding glyph size. There are currently some dimensions set to 16px (width and height) for this start slot and this is what is causing part of the issue.

From some quick live css testing in Chrome debug tool, a fix could look like:

  • remove the 16px constraint
  • add display: inline-flex; align-items: center; as styles for the start and end content.

I have no idea if this is the right path for this particular issue. Someone with a wider view of the library should ensure consistency accross components.

@chrisdholt
Copy link
Member

I'm going to put together a quick repro of this and see if we can get it into this release.

@chrisdholt
Copy link
Member

@ben-girardet I think I know where this may be coming from based on a separate investigation. I think the issue here is the fixed width being set across the controls: https://github.com/microsoft/fast/blob/master/packages/web-components/fast-components/src/menu-item/menu-item.styles.ts#L85

I think we likely don't want to fix the width here for start/end to 16px

@chrisdholt chrisdholt added status:planned Work is planned and removed status:needs-investigation Needs additional investigation labels Oct 26, 2020
@khamudom khamudom self-assigned this Oct 26, 2020
@bheston
Copy link
Collaborator

bheston commented Oct 26, 2020

From a design perspective I'm not sure removing the size is the right change to make. @ben-girardet is this case you discovered as a test only or is there a design you had in mind? I'm trying to understand what the expected behavior would be for content preceding the menu item text. Granted we intend for the slots to be flexible, but hadn't identified what that might be for this particular component which needs to align as you point out.

The reason the size is built in is so that multiple items within the menu align regardless of whether all items have start content. I'd always wanted to have support for a no-icons / start mode for uses that would benefit from less padding on the left, but that still needs to be consistent across items. This is possible to do with style overrides, so it didn't become a high priority.

As you point out, button could also become out of alignment from varying start content. What would the expectation here be though? How can the components know they are visually related to another component on the page and how much space to leave? The closest case that comes to mind is a tree view where each item might be a button with or without an icon or other start content.

@chrisdholt and @khamudom for further consideration.

@chrisdholt
Copy link
Member

From a design perspective I'm not sure removing the size is the right change to make. @ben-girardet is this case you discovered as a test only or is there a design you had in mind? I'm trying to understand what the expected behavior would be for content preceding the menu item text. Granted we intend for the slots to be flexible, but hadn't identified what that might be for this particular component which needs to align as you point out.

The reason the size is built in is so that multiple items within the menu align regardless of whether all items have start content. I'd always wanted to have support for a no-icons / start mode for uses that would benefit from less padding on the left, but that still needs to be consistent across items. This is possible to do with style overrides, so it didn't become a high priority.

I think this is going to be too prescriptive to be honest for all or any start/end content. There's a way we could build this to ensure that the text continues to align and technically we could align whatever is in start/end as well. The problem we end up with by keeping this is that a menu or menu item in the FAST components can really only support a 16x16 thing. There's a tension here between how we provide broad support and flexibility and how we can ensure that for most implementations things "just work". I think from a FAST components standpoint, we talk about this being super flexible and somewhat of an "anti-design system". If we don't support anything but 16x16 across al start/end instances, I think that's a miss on our goals.

@ben-girardet
Copy link
Contributor Author

@bheston this came to me when trying to use the fast-menu-item and placing icons (font-awesome in that case) in the start slot. I usually have icons at 24x24px inside buttons, not 16x16. Therefore I don't quite understand these fixed values.

I'm all in for consistency and providing basic useful styles in components. I guess I was surprised that using standard icons in the start slot would cause mis-alignement. It's always possible to fix this by overriding the CSS at the application level, but I'm wondering if there would not be something nicer to fix the problems you mention above.

I'm far from an expert in UX design and CSS. I mainly opened this issue because I guess I expected a better result "out of the box" when using standard icons in the start slot.

@chrisdholt
Copy link
Member

chrisdholt commented Oct 26, 2020

What about a fixed size but larger than 16px ?

The above is my primary concern to be honest. I'm not sure we have a generic enough principle to hoist these to the overall design system. Perhaps it could be a CSS variable which could set it?I think the biggest question is, what's the expected default behavior? I would expect for the goals of FAST components that I can put something in start/end and it will render in an unbroken state. By fixing all of these to 16px we are expecting one type of content being an SVG.

My current expectation here is to do the work to remove the requirement of start/end being 16px but still keep the slotted SVG styling at 16px for the time being so that the default there works as expected.

@khamudom
Copy link
Contributor

My current expectation here is to do the work to remove the requirement of start/end being 16px but still keep the slotted SVG styling at 16px for the time being so that the default there works as expected.

I think this is the right approach. I have already made the changes and made sure to test for no regression.

@khamudom khamudom reopened this Oct 26, 2020
@bheston
Copy link
Collaborator

bheston commented Oct 27, 2020

I see no issues with removing the fixed 16x16 value, as we know we plan to do to support at least 20x20 icons as well. 24x24 would fit within the relative item height as well, so no issues there.

In general I agree that we don't want to have constrained sizes, as we should remove for components like buttons. If there's a way we can adjust menus so they align correctly that sounds like the best solution. If that's not easy, or as a default, it seems we could start with reserving a square space for the start content in a menu. That would accept anything up to 32x32 within a square at the default height.

khamudom added a commit that referenced this issue Oct 29, 2020
# Description

On buttons and menu-item, the start and end slots have fixed width and height. This will cause elements larger than 16px to clip.
This PR removes the fixed dimension on the start and end slot but keeps the 16px on the SVG.
I also updated the `grid-template-columns` property on the menu-item that will also cause clipping with elements that are larger than 42px.
This should not break any current implementation when using a `button`, `anchor`, or the `menu-item`. This will now allow authors to add larger elements to the start and end slots if the design calls for it.

fixes #3904 


## Motivation & context

<!--- What problem does this change solve? -->
<!--- Provide a link if you are addressing an open issue. -->

## Issue type checklist

<!--- What type of change are you submitting? Put an x in the box that applies: -->

- [ ] **Chore**: A change that does not impact distributed packages.
- [x] **Bug fix**: A change that fixes an issue, link to the issue above.
- [ ] **New feature**: A change that adds functionality.

**Is this a breaking change?**
- [ ] This change causes current functionality to break.

<!--- If yes, describe the impact. -->

**Adding or modifying component(s) in `@microsoft/fast-components` checklist**

<!-- Do your changes add or modify components in the @microsoft/fast-components package? Put an x in the box that applies: -->

- [ ] I have added a new component
- [ ] I have modified an existing component
- [ ] I have updated the [definition file](https://github.com/Microsoft/fast/blob/master/packages/web-components/fast-components/CONTRIBUTING.md#definition)
- [ ] I have updated the [configuration file](https://github.com/Microsoft/fast/blob/master/packages/web-components/fast-components/CONTRIBUTING.md#configuration)

## Process & policy checklist

<!--- Review the list and check the boxes that apply. -->

- [ ] I have added tests for my changes.
- [x] I have tested my changes.
- [ ] I have updated the project documentation to reflect my changes.
- [x] I have read the [CONTRIBUTING](https://github.com/Microsoft/fast/blob/master/CONTRIBUTING.md) documentation and followed the [standards](https://www.fast.design/docs/community/code-of-conduct/#our-standards) for this project.

<!---
Formatting guidelines:

Accepted peer review title format:
<type>: <description>

Example titles:
    chore: add unit tests for all components
    feat: add a border radius to button
    fix: update design system to use 3px border radius

    <type> is required to be one of the following:

        - chore: A change that does not impact distributed packages.
        - fix: A change which fixes an issue.
        - feat: A that adds functionality.

    <description> is required for the CHANGELOG and speaks to what the user gets from this PR:

        - Be concise.
        - Use all lowercase characters. 
        - Use imperative, present tense (e.g. `add` not `adds`.)
        - Do not end your description with a period.
        - Avoid redundant words.

For additional information regarding working on FAST, check out our documentation site:
https://www.fast.design/docs/community/contributor-guide
-->
@EisenbergEffect EisenbergEffect added closed:done Work is finished and removed status:planned Work is planned labels Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug closed:done Work is finished community:good-first-issue Good issues for first time contributors community:request Issues specifically reported by a member of the community.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants