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

feat(toolbar): support vertical dividers through mat-divider #10528

Closed
thw0rted opened this issue Mar 22, 2018 · 13 comments
Closed

feat(toolbar): support vertical dividers through mat-divider #10528

thw0rted opened this issue Mar 22, 2018 · 13 comments
Assignees
Labels
area: material/toolbar P4 A relatively minor issue that is not relevant to core functions
Projects

Comments

@thw0rted
Copy link

thw0rted commented Mar 22, 2018

Bug, feature request, or proposal:

Bug

What is the expected behavior?

It should be possible to display a divider in a mat-toolbar per the spec:

Example image

What is the current behavior?

Placing <mat-divider [vertical]="true"> creates a 0 x 1 pixel element.

What are the steps to reproduce?

In this StackBlitz, you can see the top toolbar has the divider but it's not visible, while the bottom toolbar manually styles the divider to show up correctly.

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

Latest everything, as far as I can tell.

Is there anything else we should know?

In the StackBlitz, I have to guess at the pixel height of the divider because I can't find exactly how tall the line is supposed to be in the spec, even though they do call out the spacing between the line and the icons on either side of it. I think it's a blind spot in the spec, since neither the Structure nor the Toolbars section actually mentions the use of dividers to logically group controls.

@julianobrasil
Copy link
Contributor

Another approach: https://stackblitz.com/edit/angular-material2-issue-ec8ueb

@thw0rted
Copy link
Author

In your example, the spacing is wrong between the divider and the adjacent buttons. If I'm going to manually style stuff anyway, I don't think I'd want to add extra wrapping elements like the div you used. The point of the issue is that <mat-toolbar><button/><mat-divider/><button/></mat-toolbar> should lay out according to the spec without manual styling, and right now it does not.

@julianobrasil
Copy link
Contributor

julianobrasil commented Mar 22, 2018

Oh, it was not supposed to be a solution, just an idea of a possible workaround. But you can achieve the spacing you desire (the one you used in the second toolbar) by adding fxLayoutGap="8px" to the wrapper div: https://stackblitz.com/edit/angular-material2-issue-ec8ueb

In theory, you could also use fxLayoutAlign="start stretch" fxLayoutGap="8px" directly in <mat-toolbar> as it is a flexbox. But it would need some additional styling because stretch would force mat-divider to take all of the height and align the inner flex items to the top.

I'm a huge fan of @angular/flex-layout 😄

[edit]: maybe @CaerusKaru can give you more appropriate directions

@CaerusKaru
Copy link
Member

CaerusKaru commented Mar 22, 2018

So I think the issue is two-fold: the styles for the elements inside the toolbar is wrong, and the style for the divider is wrong. It should be something like this (though the sibling CSS might be wrong, it's the intention):

.mat-toolbar > * + .mat-divider-vertical {
	margin-right: 16px;
}

.mat-toolbar > .mat-divider-vertical + * {
	margin-left: 24px;
}

.mat-toolbar > .mat-divider-vertical {
	margin-left: -1px;
}

Does this look right to you? I'm not going to comment on the internal sizing of the button in relation to the spec, mostly because it's not defined in that example. So while it looks like it should be smaller, since it's not defined, I think it's fine.

Edited: I keep forgetting that there's no previous sibling selector, so instead of the second and third styles, maybe just something like:

.mat-toolbar > .mat-divider-vertical + * {
	margin-right: 24px;
	margin-left: -1px;
}

@thw0rted
Copy link
Author

I'm not a CSS expert but I'm not clear on why the sibling element style would be changed -- shouldn't the divider just contribute its own margin and trust the siblings to make up the rest? What if a toolbar element wants to set its own margin for whatever reason? It looks like the spec doesn't work that way, but that bothers me for some reason.

@CaerusKaru
Copy link
Member

The facts remain that this is how the spec is defined. If someone wants to override the spec margins, they are free to do so. The important thing about sibling selectors is that you don't want errant margins showing up in case someone puts a divider first or last.

I'll put together a PR and hopefully the Material Design team can comment on it with the internal guidelines on this.

@josephperrott josephperrott added the P4 A relatively minor issue that is not relevant to core functions label Mar 27, 2018
@jmls
Copy link

jmls commented May 25, 2019

was this ever fixed ? I've just hit the same problem

@ShaneLillieRAD
Copy link

@devversion any chance this is going to be fixed? I'm hitting it as well.

@qortex
Copy link

qortex commented Dec 1, 2019

Hitting it too

@devversion devversion changed the title mat-divider (vertical) doesn't work in mat-toolbar feat(toolbar): support vertical dividers through mat-divider May 25, 2020
@devversion devversion added this to Triaged in triage #1 via automation May 25, 2020
@lorenzocovarrubiasjr
Copy link

any update? @devversion

@jelbourn
Copy link
Member

Closing this since we don't have any plans to implement this feature. The toolbar is in fact unfortunately named (based on the 2014-era Material Design spec), and doesn't actually correspond to the role="toolbar" interaction pattern, where having vertical dividers like this would make more sense. The <mat-toolbar> is really just meant to be a container for titles- it's not much more than a glorified div with a background color.

@thw0rted
Copy link
Author

The spec is kind of a mess, briefly mentioning desktop in the density section then otherwise pretending it doesn't exist. Are toolbars-as-actual-toolbars just totally unsupported now? Do we have to figure out our own? Seems like a pretty big gap.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: material/toolbar P4 A relatively minor issue that is not relevant to core functions
Projects
No open projects
triage #1
  
Triaged
Development

No branches or pull requests

10 participants