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

Use CSS animation for console progress #5871

Merged
merged 1 commit into from
Nov 6, 2021

Conversation

zbynek
Copy link
Contributor

@zbynek zbynek commented Oct 31, 2021

The current spinner gif doesn't look great in dark theme or on high-res screens.

Before
old-css-animation.mov
old-animation-dark-theme.mov
After
new-spinner-animation.mov
new-animation-dark-theme.mov

Proposed changelog entries

  • Use CSS animation for console progress.

Proposed upgrade guidelines

N/A

Submitter checklist

  • [n/a] (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • [n/a] Appropriate autotests or explanation to why this change has no tests
  • [n/a] For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@timja

Screen captures

Default theme

progress-animation-default

Dark theme

progress-animation

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Love it, I've added videos to the description for others

@timja timja added web-ui The PR includes WebUI changes which may need special expertise rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Nov 1, 2021
@timja timja requested review from fqueiruga, uhafner and a team November 1, 2021 07:57
Copy link
Contributor

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

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

Pretty nice visual effect :)

(not tested manually, thanks Tim for the video)

Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

Looks much better than the existing animation, thanks!

One side note: this animation cannot be embedded into a button (see https://getbootstrap.com/docs/5.1/components/spinners/#buttons for an example) but I think we do not use such animations anyway right now (even though it might make sense in lengthy operations like in #5862 or in the update manager refresh).

Additionally, this animation is not consistent with our spinning build status icons. So we have three different progress animations on the build page now. But this is no blocker for this PR.

@daniel-beck
Copy link
Member

I think we do not use such animations anyway right now

I think I've seen progress animations in dropdown fields, does that count?


This is a great idea, and the spinner has been terrible for a while so I'm all for replacing it. 👍

That said, my first reaction when I saw the new design is that the "before" is doing something, while the "after" is just waiting. For the agent log that seems fine, but for build logs I expect confusion. Is that just me? If so I'll just get used to it 😉

@zbynek
Copy link
Contributor Author

zbynek commented Nov 1, 2021

There are some places in the forms where spinner.gif is still referenced. I think it would make sense to replace those with the new spinning wheel (like build status, just without the checkmark). For the console I thought a horizontal animation would fit better the lines being added incrementally. If there is a consensus to use spinning wheels everywhere I can do that of course.

@janfaracik
Copy link
Contributor

janfaracik commented Nov 1, 2021

There are some places in the forms where spinner.gif is still referenced. I think it would make sense to replace those with the new spinning wheel (like build status, just without the checkmark). For the console I thought a horizontal animation would fit better the lines being added incrementally. If there is a consensus to use spinning wheels everywhere I can do that of course.

I've had a branch in the works for a few days to resolve this funnily enough - I've just opened a PR here #5876

Would love to hear your thoughts on it.

Thanks

Copy link
Contributor

@fqueiruga fqueiruga left a comment

Choose a reason for hiding this comment

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

Code LGTM

Copy link
Member

@alecharp alecharp left a comment

Choose a reason for hiding this comment

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

This is really nice ! Thanks

@timja
Copy link
Member

timja commented Nov 2, 2021

So we've got 2 PRs doing pretty similar things:

#5871
#5876

Any thoughts on how we want to proceed?

@zbynek
Copy link
Contributor Author

zbynek commented Nov 2, 2021

@janfaracik your PR looks great. I'm wondering if having a different animation for dropdown/search loading and console would be a feature or just an undesired inconsistency. The reason I used the dots is to use more space horizontally, somehow the left-aligned spinner in console didn't feel right. Any thoughts?

If you think there's some value in different animations I can rebase this PR on yours, otherwise I'll just drop it.

@fqueiruga
Copy link
Contributor

I think both animations can have their place. The three dots feels right for the console, while the spinner I feel it's better for more traditional uses.

@zbynek zbynek force-pushed the console-animation branch 2 times, most recently from d14e57f to 2b08e74 Compare November 4, 2021 21:17
@zbynek
Copy link
Contributor Author

zbynek commented Nov 4, 2021

@janfaracik this is now rebased on #5876 .
@timja @uhafner @daniel-beck @Wadeck @alecharp thanks for reviewing this earlier! Any preference which animation should be used for the console?

@uhafner
Copy link
Member

uhafner commented Nov 4, 2021

I haven't yet seen #5878 in my local instance yet so I can compare it only to the current state. From your video it looks like your animation would fit better for the console log (while the other progress animation looks better for the other use cases). So I am fine with using two different ones, this one for the console and #5878 for the other ones (especially if there is not much space available).

@timja
Copy link
Member

timja commented Nov 5, 2021

@zbynek could you rebase on master?

@timja timja added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Nov 5, 2021
@timja
Copy link
Member

timja commented Nov 5, 2021

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Fine with me. CSS animation is also a good practice for such cases from what I know

@Camusensei
Copy link

Hello. I updated to a version of Jenkins which contains this change and while I love the idea (I am using a dark screen, and a CSS animation is an excellent choice for a replacement), I think that the new design is way too distracting.
I am currently just editing the page to make it disappear while I am looking at the output or I can't focus, and I will probably find a way to do it automatically (custom css rule or something).

Am I the only one who has this problem?

@timja
Copy link
Member

timja commented Jun 1, 2022

Can you provide a gif showing the issue? Better to post in https://gitter.im/jenkinsci/ux-sig or create an issue on issues.jenkins.io labelled ux, component core

@Camusensei
Copy link

Camusensei commented Jun 1, 2022

It's not an issue, it's just too big and too distracting (gifs are already at the top of this issue). I'm asking if it's just my sensibility to movement on the screen or if other people think the same.

@zbynek
Copy link
Contributor Author

zbynek commented Jun 1, 2022

@Camusensei are you setting the "prefers reduced motion" option in your OS? It would be probably good if the animation reflected this setting (via media query https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-reduced-motion ).

@Camusensei
Copy link

No I didn't. And I expect lots of animations to be lost if I do. But enough about me, I can manage with a custom CSS stylesheet to hide it or reduce it. Also, when I use dark mode, it's not that, the contrast is small so it's fine, it's really for the light version that I can't focus at all.
In any case, my question was more general: Is this as a default "spinner" good and acceptable or should we as a community consider changing it to be less distracting? That's my question :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
10 participants