Skip to content

Conversation

@bwees
Copy link
Member

@bwees bwees commented May 22, 2025

Description

Replaces CircleIconButton with IconButton from @immich/ui.

How Has This Been Tested?

This should definitely be checked over by multiple people to verify all buttons look correct. I did my best to find all buttons and verify they looked correct but there were some that I could not get to show.

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if applicable
  • I have no unrelated changes in the PR.
  • I have confirmed that any new dependencies are strictly necessary.
  • I have written tests for new code (if applicable)
  • I have followed naming conventions/patterns in the surrounding code
  • All code in src/services/ uses repositories implementations for database calls, filesystem operations, etc.
  • All code in src/repositories/ is pretty basic/simple and does not have any immich specific logic (that belongs in src/services/)

@github-actions
Copy link
Contributor

Deploying preview environment to https://pr-18486.preview.internal.immich.cloud/

@bwees bwees force-pushed the chore/migrate-circle-icon-button branch from 9e6087f to a758ca7 Compare May 22, 2025 21:12
@bwees bwees marked this pull request as ready for review May 22, 2025 21:13
@bo0tzz bo0tzz removed the preview label May 23, 2025
@alextran1502
Copy link
Member

A couple of issues

  1. (Photos/Favorite/Archive) page, select assets, wrong color on <ButtonContextMenu/>
image
  1. Generally looks good in dark theme, but there is a problem with the light theme in asset viewer
image

@jrasm91 We probably need an option to force dark or light for these buttons for correct behavior in some views that always stay dark, thoughts?

@jrasm91
Copy link
Member

jrasm91 commented May 23, 2025

I guess we could add a theme prop that's light or dark and then:

  • add light to the body when it is light mode
  • add a light" or "dark" class to the button when the theme is specified

@bwees bwees force-pushed the chore/migrate-circle-icon-button branch 2 times, most recently from c4ef70a to 4bd468a Compare May 26, 2025 18:36
@alextran1502
Copy link
Member

Hi Brandon, can you help resolve the merge conflict and then ping me? I will run another round of tests and get it in

@bwees bwees force-pushed the chore/migrate-circle-icon-button branch from 11dd3cc to 22e9d8e Compare May 30, 2025 22:49
@bwees bwees requested review from alextran1502 and danieldietzler and removed request for alextran1502 May 30, 2025 23:19
@bwees bwees force-pushed the chore/migrate-circle-icon-button branch from e042fa5 to 0285bc8 Compare May 31, 2025 17:01
@alextran1502
Copy link
Member

A couple more

  • In light mode, the memory-viewer buttons in control bar need to be forced dark
image - The back button in asset-viewer also needs to be forced dark image

Those are the two places I noticed. Please turn to light mode and check other pages as well. Thank you

@bwees bwees force-pushed the chore/migrate-circle-icon-button branch from 8f1c378 to ba8a2ad Compare June 2, 2025 13:48
@alextran1502 alextran1502 enabled auto-merge (squash) June 2, 2025 14:42
@alextran1502 alextran1502 merged commit a02e1f5 into main Jun 2, 2025
51 checks passed
@alextran1502 alextran1502 deleted the chore/migrate-circle-icon-button branch June 2, 2025 14:47
@github-actions github-actions bot removed the preview label Jun 2, 2025
savely-krasovsky pushed a commit to savely-krasovsky/immich that referenced this pull request Jun 8, 2025
…-app#18486)

* remove import and referenced file

* first pass at replacing all CircleIconButtons

* fix linting issues

* fix combobox formatting issues

* fix button context menu coloring

* remove circle icon button from search history box

* use theme switcher from UI lib

* dark mode force the asset viewer icons

* fix forced dark mode icons

* dark mode memory viewer icons

* fix: back button in memory viewer

---------

Co-authored-by: Alex <alex.tran1502@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants