-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat(board): allow to set icon color of widgets #2228
Conversation
Added a new color option to the board to allow picking an icon color Added new css theme variable for the icon color Updated app, bookmarks and dns-hole>controls widgets to take this new color into account when displaying icons Leaving the color field empty, will use the respective icon colors
Here's the code health analysis summary for commits Analysis Summary
|
Added the required `theme.other` type to the relevant `tsconfig.json` files Allows to remove previous type casting of `theme.other.hasIconColor`
Effected had been removed by accident in a previous commit Clarified usage of css class only for when it has an Url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some other things to do:
- Drop db migrations 23 / 24 (in packages/db run
pnpm migration:sqlite:drop
(and mysql) and select the migrations to drop - Move the theme override to @homarr/ui and remove the types package
- I'll also take a look regarding the types if there is a better way to automatically make it work in the whole monorepository
There only one 23 showing when I try to drop both sqlite or mysql. Do I just remove the files manually? |
No removing the files manually will probably break as it also modifies the |
… with `.or(z.literal(""))`
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as duplicate.
This comment was marked as duplicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should all be sorted now, just a question:
- I've noticed someone used
const board = useRequiredBoard()
on the app widget, would it be worth to change thetheme.other.hasIconColor
on the theme (which we are trying to find a way to cascade the type around all the projects) to use this variable and go straight to the board properties to see if a color has been defined?
Also, please note since I've dropped the migrations I've not generated a new one with the changes to the board scheme, I'm guessing that is done when merging to dev
but let me know if you want me run them.
Yeah maybe it is a good idea to not have this type hacks in the project and just use the |
Co-authored-by: Crowdin Homarr <190541745+homarr-crowdin[bot]@users.noreply.github.com>
Co-authored-by: homarr-renovate[bot] <158783068+homarr-renovate[bot]@users.noreply.github.com>
Tear when i são that as was thinking the same. I will try to make those changes this weekend, they shouldn't take long. |
This comment was marked as duplicate.
This comment was marked as duplicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should all be done!
I'll take a look at it tomorrow evening and will also fix the merge conflicts if you want 👍🏼 Thank you for your work ❤️ |
Just happy to help. I've aaceepted all incoming changes on the migrations and re-generated the required ones, should be sorted but feel free to let me know if not :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this nice feature! I already look forward to the upcoming show-offs
Done this by leveraging the css mantine theme library, by adding a new
iconColour
color option plus anhasIconColor
in theother
section, so it can default to the default app icon. Not sure if this would be the best way to do it or you would have a better way to achieve this, I can adjust it if needed.Homarr
Thank you for your contribution. Please ensure that your pull request meets the following pull request:
pnpm buid
, autofix withpnpm format:fix
)dev
branchx
,y
,i
or any abbrevation)