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.

Sign up for GitHub

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

Jump to bottom

feat(Header): Add the condition if center section is not shown #196

Merged
merged 1 commit into from
Jun 10, 2024
Merged

feat(Header): Add the condition if center section is not shown #196

merged 1 commit into from
Jun 10, 2024

Conversation

lialila
Copy link
Contributor

@lialila lialila commented Jun 10, 2024

No description provided.

@vercel Vercel
Copy link

vercel bot commented Jun 10, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
boemly ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 10, 2024 1:53pm

@lialila lialila changed the title Update header fix(Header): Make left, right and center width customisable Jun 10, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 10, 2024 •
edited
Loading

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/boemly.cjs.production.min.js 1.56 MB (0%) 32 s (0%) 3.3 s (-3.07% 🔽) 35.2 s
dist/boemly.esm.js 72.83 KB (0%) 1.5 s (0%) 366 ms (-0.41% 🔽) 1.9 s

lukasbals
lukasbals requested changes Jun 10, 2024
View reviewed changes
Copy link
Member

@lukasbals lukasbals left a comment

Choose a reason for hiding this comment

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

Hmmm - some thoughts.

  1. This is no fix but a new feature.
  2. Wouldn't it work if you just put the header contents into right and leave center empty? Is it then too narrow?

lukasbals
lukasbals reviewed Jun 10, 2024
View reviewed changes
Copy link
Member

@lukasbals lukasbals left a comment

Choose a reason for hiding this comment

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

How did that work with 76% as the left width? 🤔

@lialila
Copy link
Contributor Author

lialila commented Jun 10, 2024

Hmmm - some thoughts.

  1. This is no fix but a new feature.
  2. Wouldn't it work if you just put the header contents into right and leave center empty? Is it then too narrow?

The text is squeezed then, that's why it is broken on two lines
Screenshot 2024-06-10 at 13 53 07

@lukasbals
Copy link
Member

Hmmm - some thoughts.

  1. This is no fix but a new feature.
  2. Wouldn't it work if you just put the header contents into right and leave center empty? Is it then too narrow?

The text is squeezed then, that's why it is broken on two lines Screenshot 2024-06-10 at 13 53 07

What about having the option to configure if center is shown? And if this is false the center is not shown and the space for left and right is 50/50?

@lialila lialila force-pushed the uc/update-the-header branch 3 times, most recently from 9f1f168 to ff98341 Compare June 10, 2024 12:31
@lialila lialila changed the title fix(Header): Make left, right and center width customisable feat(Header): Add the condition if center section is not shown Jun 10, 2024
lukasbals
lukasbals reviewed Jun 10, 2024
View reviewed changes
Copy link
Member

@lukasbals lukasbals left a comment

Choose a reason for hiding this comment

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

Nice work. Can you also cover this in a test case, please?

@lialila lialila requested a review from raulsteurer June 10, 2024 13:36
lukasbals
lukasbals requested changes Jun 10, 2024
View reviewed changes
src/components/Header/Header.test.tsx Outdated Show resolved Hide resolved
src/components/Header/Header.test.tsx Outdated Show resolved Hide resolved
lukasbals
lukasbals approved these changes Jun 10, 2024
View reviewed changes
Copy link
Member

@lukasbals lukasbals left a comment

Choose a reason for hiding this comment

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

Nice

@lialila lialila merged commit b34266c into main Jun 10, 2024
7 checks passed
@lialila lialila deleted the uc/update-the-header branch June 10, 2024 13:57
@github-actions GitHub Actions
Copy link

🎉 This PR is included in version 5.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukasbals lukasbals lukasbals approved these changes

@rschiffer rschiffer Awaiting requested review from rschiffer

@raulsteurer raulsteurer Awaiting requested review from raulsteurer

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lialila @lukasbals @raulsteurer