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(ui): Unscrollable error message when saving YAML (#4152) #4195

Merged
merged 7 commits into from
Apr 29, 2022

Conversation

rbreeze
Copy link
Member

@rbreeze rbreeze commented Aug 28, 2020

Closes #4152

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Optional. My organization is added to USERS.md.
  • I've signed the CLA and my build is green (troubleshooting builds).

@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2020

Codecov Report

Merging #4195 (da0218a) into master (1533698) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4195      +/-   ##
==========================================
- Coverage   45.73%   45.71%   -0.02%     
==========================================
  Files         220      220              
  Lines       26006    26006              
==========================================
- Hits        11893    11888       -5     
- Misses      12456    12460       +4     
- Partials     1657     1658       +1     
Impacted Files Coverage Δ
applicationset/services/scm_provider/github.go 75.29% <0.00%> (-5.89%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1533698...da0218a. Read the comment docs.

@rbreeze rbreeze requested a review from alexmt August 28, 2020 21:36
@RichiCoder1
Copy link

Considering it's possible that this error could contain actionable or verbose info, would it be possible to move this information out of a notification and put it inline? Maybe in a collapsable, with the notification just being a more present indicator that something failed and link to the div containing error information

@rbreeze
Copy link
Member Author

rbreeze commented Aug 30, 2020

@RichiCoder1 This issue made me think of this as well. I think some of the error messaging could benefit from a better design.

@RichiCoder1
Copy link

@rbreeze Currently spinning up a local dev env (and attempting to recruit my UX Designer BF) so I can assist with that. I think for this specifically there's a easy fix, I'll see if I can throw together an example or design.

@rbreeze rbreeze linked an issue Oct 20, 2020 that may be closed by this pull request
3 tasks
@rbreeze rbreeze requested a review from jsoref January 13, 2021 00:33
@rbreeze rbreeze linked an issue Jan 13, 2021 that may be closed by this pull request
3 tasks
@rbreeze rbreeze added the only-needs-a-rubber-stamp The author thinks this is low-risk & low-impact, so suggest a cursory review label Jan 20, 2021
Copy link
Member

@jsoref jsoref left a comment

Choose a reason for hiding this comment

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

rubber stamp 💮

@crenshaw-dev crenshaw-dev changed the title fix(ui): Unscrollable error message when saving YAML. Closes #4152 fix(ui): Unscrollable error message when saving YAML (#4152) Apr 10, 2022
@crenshaw-dev
Copy link
Member

@rbreeze can you fix DCO on this?

Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

Can you please attach a screenshot?

@pasha-codefresh
Copy link
Member

pasha-codefresh commented Apr 29, 2022

After fix :
Снимок экрана 2022-04-29 в 19 56 52

Before:
Снимок экрана 2022-04-29 в 19 54 44
And not scroll

Actually i like this improvement, but also would be nice apply it to all error notifications, maybe wrap in some component

@pasha-codefresh
Copy link
Member

wdyt @alexmt @crenshaw-dev , for me it looks good

Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

lgtm!

@rbreeze rbreeze enabled auto-merge (squash) April 29, 2022 17:54
@rbreeze rbreeze merged commit 40a4061 into argoproj:master Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
only-needs-a-rubber-stamp The author thinks this is low-risk & low-impact, so suggest a cursory review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error message truncated in UI when it is very long Error message truncated in UI when it is very long
7 participants