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(notification): fit long words #546

Merged

Conversation

JessieTeng89
Copy link
Contributor

@JessieTeng89 JessieTeng89 commented Apr 5, 2024

Fixes #512
It is a layout issue when there is a long word in the error message toast popup, so added a style in that component to fix the issue.

Screenshots:
before fix:
image

after fix:
image

@JessieTeng89 JessieTeng89 changed the title bugfix-512: error message layout bugfix: error message layout Apr 5, 2024
@agilgur5 agilgur5 changed the title bugfix: error message layout fix: fit error notification Apr 5, 2024
@agilgur5 agilgur5 changed the title fix: fit error notification fix: fit long words in error notification Apr 5, 2024
@JessieTeng89 JessieTeng89 marked this pull request as ready for review April 20, 2024 13:39
@JessieTeng89
Copy link
Contributor Author

@crenshaw-dev Kindly help to review this PR

@JessieTeng89
Copy link
Contributor Author

JessieTeng89 commented May 2, 2024

Hi @crenshaw-dev @jmeridth @terrytangyuan @rbreeze
Much appreciate if you can help to review it or let me know if any other actions needed before merging it?
Thanks!

@JessieTeng89
Copy link
Contributor Author

Hi @agilgur5
A gentle follow up with this PR, could you please help to review it?

@agilgur5
Copy link
Contributor

agilgur5 commented Jun 11, 2024

Yes, I had been planning to; I'm not sure why you asked several other uninvolved people for review a day after you made changes, which was a full 2 weeks after changes were requested.

Please be patient, especially if you yourself do not respond immediately -- it would not be surprising if a maintainer took as long as you did to respond if not longer -- I don't think it's reasonable to expect maintainers to be faster than you yourself.
And please interact with maintainers who already reviewed your PR, instead of asking others, which is counter-productive.

Copy link
Contributor

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

There are simpler ways of achieving this without overriding the component's styles, which can cause more problems and is not very reliable

src/components/notifications/notifications.scss Outdated Show resolved Hide resolved
src/components/notifications/notifications.tsx Outdated Show resolved Hide resolved
@agilgur5 agilgur5 changed the title fix: fit long words in error notification fix(notification): fit long words Jun 11, 2024
JessieTeng89 and others added 5 commits June 21, 2024 09:18
Signed-off-by: Jessie <yilin.teng@fmr.com>
Signed-off-by: Jessie <yilin.teng@fmr.com>
Signed-off-by: Jessie <yilin.teng@fmr.com>
Signed-off-by: Jessie <yilin.teng@fmr.com>
Signed-off-by: Jessie Teng <jessie.teng@fmr.com>
@JessieTeng89
Copy link
Contributor Author

Hi @agilgur5
I just updated the change based on your suggestion, sorry for the delay and Thanks for your support!

@agilgur5 agilgur5 self-assigned this Jun 22, 2024
Signed-off-by: Jessie Teng <jessie.teng@fmr.com>
Copy link
Contributor

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for noting and fixing this issue

@agilgur5 agilgur5 enabled auto-merge (squash) June 26, 2024 23:03
@agilgur5 agilgur5 merged commit 7246755 into argoproj:master Jun 26, 2024
5 checks passed
@gillesbouvier-qz
Copy link

gillesbouvier-qz commented Jul 14, 2024

Did this change add the following CSS?

.Toastify__toast-body > div:last-child {
  word-break: break-word;
  ...
}

If so, it is not working as expected as it is breaking short words too. Did you mean to use this instead?

    overflow-wrap: break-word;

Note: In contrast to word-break, overflow-wrap will only create a break if an entire word cannot be placed on its own line without overflowing.

@agilgur5
Copy link
Contributor

agilgur5 commented Jul 14, 2024

No, you can see the diff for yourself here in the PR, which is all of 4 LoC of an in-line style that uses overflow-wrap: break-word.

word-break: break-word is also deprecated, which I pointed out in my review of the very first iteration

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.

UI error notification doesn't fit
3 participants