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

Blockbase: remove hardcoded flex justify content CSS rule for header component #5093

Closed
wants to merge 1 commit into from

Conversation

matiasbenedetto
Copy link
Member

Changes proposed in this Pull Request:

Blockbase: remove hardcoded flex justify content CSS rule for header component

Related issue(s):

#5078

scruffian
scruffian previously approved these changes Nov 24, 2021
Copy link
Member

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

I tested videomaker and blockbase. LGTM.

@mikachan
Copy link
Member

I've tested this on Blockbase and I can't align the items in the header row, but I'm not sure if I'm doing something different. I'm testing on this branch with the latest GB trunk. Here's what I'm doing:

header.alignment.mov

It looks like if I also remove flex-grow: 1; from header.scss, then I can apply alignment to the row:

header.alignment.2.mov

But I think by default we want the navigation to be right-aligned, and the other items on the left (at least in Blockbase). I'm not sure how to achieve this with the GB options! I'll have a think.

@MaggieCabrera
Copy link
Contributor

I made an attempt at this over at #5102 and I ended up removing a lot of unneeded CSS but to make the behavior the same as the original menu I had to add a flex-basis: 100% that renders the justification attributes useless too... Headers are hard!!!

@MaggieCabrera
Copy link
Contributor

I'm going to step out from this for a bit and work on other stuff but I think the key here is to remove most of the extra CSS we are adding to achieve the desktop layout and add whatever CSS is needed just to fix the responsive view. That way the user can still tinker in the editor successfully while we keep the responsive version consistent with the original design.

@scruffian scruffian dismissed their stale review November 25, 2021 11:24

there were better reivews!

@matiasbenedetto
Copy link
Member Author

Closing this PR in favor of #5120

@scruffian scruffian deleted the fix/5078 branch November 30, 2021 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants