streamlit icon indicating copy to clipboard operation
streamlit copied to clipboard

Make the running man icon match theme colors

Open TreavVasu opened this issue 6 months ago • 15 comments

Describe your changes

This PR replaces the static icon_running.gif image used in the StatusWidget component with a dynamic, theme-aware React component called IconRunning.

The gif was collection of icons image

Using those Icons from icon library implementing this:

https://github.com/user-attachments/assets/b7f8784e-920c-4650-8b90-3961c896595e

GitHub Issue Link (if applicable)

#11371

Testing Plan

  • Explanation of why no additional tests are needed
  • Unit Tests (JS and/or Python)
  • E2E Tests
  • Any manual testing needed?

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

TreavVasu avatar May 27 '25 12:05 TreavVasu

:tada: Snyk checks have passed. No issues have been found so far.

:white_check_mark: security/snyk check is complete. No issues have been found. (View Details)

:white_check_mark: license/snyk check is complete. No issues have been found. (View Details)

snyk-io[bot] avatar May 27 '25 12:05 snyk-io[bot]

Hey @TreavVasu! Awesome, thanks for your contribution! The animation in your video looks pretty slow and the icon also seems to be bigger than today – any chance we can match this to what we have today? Also, the color should be the same as the icon that opens/closes the sidebar ideally.

cc @sfc-gh-nbellante since he's also working on some changes to the running man due to the top navigation bar we're adding, just to avoid any more merge conflicts ;)

jrieke avatar May 27 '25 18:05 jrieke

Sure @jrieke I'll try to match that and also keep the color in check

TreavVasu avatar May 27 '25 18:05 TreavVasu

@jrieke

  • Size: Same as in declared variables.
  • Color: Fixed for Light theme, there is issue for dark theme would try to resolve
  • Have made changes on speed based on gif value

image

Currently set at 200ms same as gif

Here's the updated version of video:

https://github.com/user-attachments/assets/f3c5b3b1-5a56-4fe1-9be1-72bedcdf204a

TreavVasu avatar May 27 '25 21:05 TreavVasu

@jrieke
Size: Same as in configs Color: Same as Navbar button that show/hides: Opacity has been set to 0.4 which was the earlier setting which makes the runnning man a bit grey in dark mode can be adjusted by modifying

image

Current Showcase of how it is rendering :

https://github.com/user-attachments/assets/86b5cbe6-9e8f-4f72-8cc0-ba7a0e918964

All the requirements should be met by this PR. Let me know if any other +- is required

TreavVasu avatar May 28 '25 15:05 TreavVasu

(I'm shifting my apartment) would add the relevant fixes wherever required tomorrow

TreavVasu avatar Jun 01 '25 04:06 TreavVasu

@lukasmasuch can you call copilot again?

TreavVasu avatar Jun 01 '25 23:06 TreavVasu

https://github.com/user-attachments/assets/c2241151-5d73-47b9-98ff-f80ef97a8395

TreavVasu avatar Jun 02 '25 07:06 TreavVasu

https://github.com/user-attachments/assets/ebb9a336-5acf-4cc2-a731-7d67fc3fd504

TreavVasu avatar Jun 02 '25 16:06 TreavVasu

@lukasmasuch One more call ig this should be last

TreavVasu avatar Jun 02 '25 16:06 TreavVasu

@jrieke can you also check once?

TreavVasu avatar Jun 03 '25 19:06 TreavVasu

@TreavVasu there is a prettier formatting issue that needs to be resolved and it seems the PR is causing unexpected frontend errors in the browser console: https://github.com/streamlit/streamlit/actions/runs/15469352188/job/43580859020?pr=11461#step:8:6984

lukasmasuch avatar Jun 09 '25 13:06 lukasmasuch

@lukasmasuch thanks for pointing out I'll look into it asap

TreavVasu avatar Jun 09 '25 13:06 TreavVasu

@TreavVasu I just tried this out locally. In light mode, the color of the running man and the arrow icon to open the sidebar match (#83858c). But in dark mode, I'm getting different values: #9fa0a2 for the running man and #fafafa for the arrow. Am I doing something wrong?

Also, @sfc-gh-nbellante, since this PR is touching the top bar and running man icon, which you changed in the top nav bar PR, would you rather want us to hold off on merging this until after top nav is merged?

jrieke avatar Jun 12 '25 00:06 jrieke

It now is set to same values that sidebar button inherits

TreavVasu avatar Jun 15 '25 18:06 TreavVasu

@TreavVasu sorry for going back and forth but I just talked with our designer (cc @shamis15) and ideally we want:

  • The running man icon to always be fadedText60.
  • The STOP text to be bodyText (note that we recently got rid of the RUNNING text in a PR we merged last week).

Also, @lukasmasuch Jessi said the arrow icons to collapse/expand the sidebar should always be fadedText60 but there's something in the code here, which switches it to bodyText in dark themes, not sure if that's a bug or it was intentional? You apparently last edited that part.

jrieke avatar Jun 17 '25 14:06 jrieke

@TreavVasu sorry for going back and forth but I just talked with our designer (cc @shamis15) and ideally we want:

  • The running man icon to always be fadedText60.
  • The STOP text to be bodyText (note that we recently got rid of the RUNNING text in a PR we merged last week).

Also, @lukasmasuch Jessi said the arrow icons to collapse/expand the sidebar should always be fadedText60 but there's something in the code here, which switches it to bodyText in dark themes, not sure if that's a bug or it was intentional? You apparently last edited that part.

Okay, no issues would love to do it properly and ensure the PR is good in every way, That aside: The update I need to make is just to set it at fadedText60 for any case and I need to make it so for both themes light and dark?

I'll update the part about recent elimination of 'Running' Text from dev branch (if it's already not updated )

TLdr: Work Items for me

  • Update the color to be just fadedText60 irrespective of theme
  • Ensure the latest merged changes are included in here

TreavVasu avatar Jun 17 '25 18:06 TreavVasu

Correct, fadedText60 for the running man icon and bodyText for the STOP text, no matter which theme is active. The deletion of RUNNING is already on develop!

jrieke avatar Jun 18 '25 00:06 jrieke

Hi @jrieke
I have added those changes, do let me know if it's suitable (I can add video of it running here too ) Also do unit test require to be in this PR too? I have yet to update old ones

TreavVasu avatar Jun 26 '25 04:06 TreavVasu