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

Force flex layout #57

Closed
jouni opened this issue Nov 16, 2018 · 7 comments
Closed

Force flex layout #57

jouni opened this issue Nov 16, 2018 · 7 comments

Comments

@jouni
Copy link
Member

jouni commented Nov 16, 2018

App Layout makes assumes that the host element is always display: flex.

If a user uses <vaadin-app-layout> as the root element of a design in Vaadin Designer, the tool will override the host with display: block, causing unexpected results.

We could prevent this by enforcing display: flex !important.

alvarezguille added a commit that referenced this issue Nov 26, 2018
@alexberazouski
Copy link

The issue is not reproduced. Designer by default sets display block only for the parent element, and that doesn't break app-layout.

The described issue happened by manual overriding display value via theme-for="vaadin-app-layout" module.

@jouni Should we anyway set it important?

@jouni
Copy link
Member Author

jouni commented Nov 26, 2018

@emarc, can you add more detail to this issue? How did you end up in the situation that app-layout was display: block?

@emarc
Copy link

emarc commented Nov 27, 2018

The issue was indeed the sum of a number of things:
Vaadin Designer does not allow multiple <dom-module> in one file, so I made my main-layout dom-module (in main-layout.html) into a theme-for in order to be able to work around the limitation with 100% height in <vaadin-app-layout>. However, Designer automatically puts :host { display:block } in each file, so I inadvertently applied that to <vaadin-app-layout> before realising (later, the bug was not obvious at first).

The reason I decided to apply the theme-for on the main-layout because 1) that's where I needed it, 2) I wanted to minimize the amount of files for my particular case, 3) the Project Base did not come with a suitable file (shared-styles?) and 4) I don't actually know what the correct place is or if it's ok to do it this way.

tl;dr vaadin-app-layout requiring theme-for + no shared-styles in Project Base + lazy programmer + Designer adding display:block + Designer not allowing multiple dom-module = bugging vaadin-app-layout.

Quite a sequence of events in my case... vaadin-app-layout could still make it harder for me to shoot myself in the foot, by making it hard to change things that should not be changed. The way the bug manifested itself (later, only at the smaller breakpoint, making it hard to find) speaks for fixing, while the fact that I have to somehow apply display:block makes this quite an edge case and speaks against fixing.

alvarezguille added a commit that referenced this issue Nov 27, 2018
@alvarezguille
Copy link
Member

I made my main-layout dom-module (in main-layout.html) into a theme-for in order to be able to work around the limitation with 100% height in

Luckily such limitation no longer exists, it was fixed in #56

could still make it harder for me to shoot myself in the foot, by making it hard to change things that should not be changed

We did this as part of #59

Thanks for the feedback and the detailed explanation, hopefully these small changes will save some time and surprises

@jouni
Copy link
Member Author

jouni commented Nov 30, 2018

Just realized, that having display: ... !important; means that the component can’t be hidden using display: none;. Probably not a relevant use case for app layout, but for some other components very much a critical use case.

@lkraav
Copy link

lkraav commented Nov 30, 2018

You probably mean "cannot be hidden"? Indeed, !important always has fun side effects.

EDIT lol I like how I pointed out your typo and then wrote "ben (be)" myself

@jouni
Copy link
Member Author

jouni commented Nov 30, 2018

Yes, meant to write “can’t”. Fixed now.

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

No branches or pull requests

5 participants