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

fix: Fix Notification title word break #15008

Merged
merged 3 commits into from
Apr 17, 2019

Conversation

iamkun
Copy link
Member

@iamkun iamkun commented Apr 8, 2019

close #14893
Please make sure these boxes are checked before submitting your PR, thank you!

  • Make sure you follow Element's contributing guide (中文 | English | Español | Français).
  • Make sure you are merging your commits to dev branch.
  • Add some descriptions and refer relative issues for you PR.

@iamkun iamkun added this to the 2.8.0 milestone Apr 8, 2019
@element-bot
Copy link
Member

element-bot commented Apr 8, 2019

Deploy preview for element ready!

Built with commit 5c176a1

https://deploy-preview-15008--element.netlify.com

@breadadams
Copy link
Contributor

@iamkun, I don't think that this is an appropriate fix. You can't just split words up for everyone's notifications. It looks bad, and I know my client will complain about this.

The correct solution would be to adjust the padding so that the text never reaches the X. I can sort this is if you want. Please re-open my original issue.

@iamkun
Copy link
Member Author

iamkun commented Apr 9, 2019

@breadadams agreed

@breadadams
Copy link
Contributor

Cool @iamkun, it's looking better! However given the right amount of characters the text could still run just against the side of the ✖️ icon:

image

I'd add ~8px of padding-right to .el-notification__title, leaving the main notifications padding as is.

@iamkun
Copy link
Member Author

iamkun commented Apr 9, 2019

I think this 2px fix would be enough for most cases. 😬

@breadadams
Copy link
Contributor

Sure thing @iamkun, could be...

I couldn't actually get it to happen with anything in english other than random characters, however a client of mine managed to with a Spanish string. Let's see how the 2px fix works and if necessary I'll open a hotfix 😉

@wacky6
Copy link
Contributor

wacky6 commented Apr 9, 2019

How about margin-right on .el-notification-group ?

image


title is optional. When omitted, notification text can still be too close to the button.

image

@breadadams
Copy link
Contributor

breadadams commented Apr 9, 2019

Very good point @wacky6, I'd never tried a notification without a title.

The right padding needs increasing a bit more. From a UI point of view I think it's better with a bigger gap like in the above screenshot, it separates the concerns of the "content" and the "action area" better (imo). Currently the close button is closer to the text than the icon, I had a client trying to click the red X icon because they didn't see the close button - true story.

So maybe changing from 28px38px?

@iamkun
Copy link
Member Author

iamkun commented Apr 10, 2019

@breadadams @wacky6 Updated based on reviews. 😀

Copy link
Contributor

@breadadams breadadams left a comment

Choose a reason for hiding this comment

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

Looks good to me @iamkun 👏

@ziyoung ziyoung merged commit f02a849 into ElemeFE:dev Apr 17, 2019
@iamkun iamkun deleted the fix/Notification_title_spacing branch May 6, 2019 07:02
weisiren168 pushed a commit to weisiren168/element that referenced this pull request Jun 20, 2019
lzq4047 pushed a commit to lzq4047/element that referenced this pull request May 22, 2020
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

Successfully merging this pull request may close these issues.

[Bug Report] Notification spacing
5 participants