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

feat(board): allow to set icon color of widgets #2228

Merged
merged 40 commits into from
Feb 19, 2025

Conversation

Aandree5
Copy link
Contributor

@Aandree5 Aandree5 commented Feb 3, 2025

  • 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

Done this by leveraging the css mantine theme library, by adding a new iconColour color option plus an hasIconColor in the other 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:

  • Builds without warnings or errors (pnpm buid, autofix with pnpm format:fix)
  • Pull request targets dev branch
  • Commits follow the conventional commits guideline
  • No shorthand variable names are used (eg. x, y, i or any abbrevation)

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
@Aandree5 Aandree5 requested a review from a team as a code owner February 3, 2025 14:14
Copy link

deepsource-io bot commented Feb 3, 2025

Here's the code health analysis summary for commits 3cca024..fbbff6b. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource JavaScript LogoJavaScript✅ Success
🎯 1 occurence resolved
View Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

Andre Silva and others added 6 commits February 4, 2025 12:00
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
Copy link
Member

@Meierschlumpf Meierschlumpf left a 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:

  1. Drop db migrations 23 / 24 (in packages/db run pnpm migration:sqlite:drop (and mysql) and select the migrations to drop
  2. Move the theme override to @homarr/ui and remove the types package
  3. I'll also take a look regarding the types if there is a better way to automatically make it work in the whole monorepository

@Meierschlumpf Meierschlumpf added the enhancement New feature or request label Feb 7, 2025
@Aandree5
Copy link
Contributor Author

Aandree5 commented Feb 8, 2025

Some other things to do:

  1. Drop db migrations 23 / 24 (in packages/db run pnpm migration:sqlite:drop (and mysql) and select the migrations to drop
  2. Move the theme override to @homarr/ui and remove the types package
  3. 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?

@Meierschlumpf
Copy link
Member

No removing the files manually will probably break as it also modifies the _journal.json
Yes you only have one 23 showing for sqlite and one showing for mysql. After you've removed it you can then also merge the changes from dev (where some db migrations were made)

@Aandree5

This comment was marked as outdated.

@Aandree5

This comment was marked as duplicate.

Copy link
Contributor Author

@Aandree5 Aandree5 left a 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 the theme.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.

@Meierschlumpf
Copy link
Member

Meierschlumpf commented Feb 14, 2025

Yeah maybe it is a good idea to not have this type hacks in the project and just use the useRequiredBoard hook 👍🏼
Once you have done that, just create a new migration for sqlite & mysql as the other stuff seems to be ready to me 😄
Sorry for not responding for a while...

homarr-crowdin bot and others added 2 commits February 15, 2025 00:44
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>
@Aandree5
Copy link
Contributor Author

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.
It's fine don't worry! I can see that the project is busy, which is good!

@Aandree5

This comment was marked as duplicate.

Copy link
Contributor Author

@Aandree5 Aandree5 left a 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!

@Meierschlumpf
Copy link
Member

I'll take a look at it tomorrow evening and will also fix the merge conflicts if you want 👍🏼 Thank you for your work ❤️

@Aandree5
Copy link
Contributor Author

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 :)

Copy link
Member

@Meierschlumpf Meierschlumpf left a 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

@Meierschlumpf Meierschlumpf changed the title feat: allow to set icon color of widgets, per board feat(board): allow to set icon color of widgets Feb 19, 2025
@Meierschlumpf Meierschlumpf merged commit de5c34a into homarr-labs:dev Feb 19, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants