Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: do not wrap header content, wrap long title #4310

Merged
merged 3 commits into from
Aug 9, 2022

Conversation

web-padawan
Copy link
Member

Description

Fixes #4309

Type of change

  • Bugfix

@rolfsmeds
Copy link
Contributor

Also note the latter part of #4309, about header-content getting pushed outside the dialog

@web-padawan
Copy link
Member Author

@rolfsmeds Updated the PR to also break a long single word in header title.

@rolfsmeds
Copy link
Contributor

@rolfsmeds Updated the PR to also break a long single word in header title.

Need to test that this does not cause unnecessary word-breaking (e.g. breaks word instead of making Dialog bigger when there's room for it)

I'm frankly a teeny bit nervous about word-breaking by default. I'd have been fine with just clipping and text-overflow:ellipsis to ensure that it doesn't push header-content outside.

Perhaps @jouni has an opinion on the matter.

@jouni
Copy link
Member

jouni commented Aug 8, 2022

breaks word instead of making Dialog bigger when there's room for it

Could this actually happen in some situation? I assume no, and I think then it would be okay to break the word instead of clipping it.

@rolfsmeds
Copy link
Contributor

breaks word instead of making Dialog bigger when there's room for it

Could this actually happen in some situation? I assume no, and I think then it would be okay to break the word instead of clipping it.

My hunch is no, but I'm never entirely sure how browsers decide between wrap/shrink elements and make the container bigger to fit them. Thus the need to test.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sissbruecker
Copy link
Contributor

Need to test that this does not cause unnecessary word-breaking (e.g. breaks word instead of making Dialog bigger when there's room for it)

Tested this, it's not an issue, the dialog will expand first before breaking words.

@web-padawan web-padawan removed the request for review from jouni August 9, 2022 08:25
@web-padawan web-padawan changed the title fix: do not wrap dialog header content fix: do not wrap header content, wrap long title Aug 9, 2022
@web-padawan web-padawan merged commit d24f64f into master Aug 9, 2022
@web-padawan web-padawan deleted the fix/dialog-header-nowrap branch August 9, 2022 08:27
@vaadin-bot
Copy link
Collaborator

Hi @web-padawan , this commit cannot be picked to 23.1 by this bot, can you take a look and pick it manually?
Error Message: Error: Command failed: git cherry-pick d24f64f
error: could not apply d24f64f... fix: do not wrap header content, wrap long title (#4310)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 23.2.0.beta1 and is also targeting the upcoming stable 23.2.0 version.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dialog header content wraps below title, or is pushed outside dialog
5 participants