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: added bordercolor option for dynamic section #2334

Merged
merged 31 commits into from
Mar 2, 2025

Conversation

jackwellerreal
Copy link
Contributor


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)

closes #2235

@jackwellerreal jackwellerreal requested a review from a team as a code owner February 15, 2025 06:32
Copy link

deepsource-io bot commented Feb 15, 2025

Here's the code health analysis summary for commits d9ef1b0..a47b3e1. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource JavaScript LogoJavaScript✅ SuccessView Check ↗

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

@Meierschlumpf
Copy link
Member

Hey
I would suggest another way to implement this:

  • Store the values in the database as superjson, which also allows to save more complex datatypes like dates, maps, etc.
  • Instead of overwriting the type with Extract<Section, { kind: "dynamic" }> & { options: object };, add the options to the validation schema (shared.ts). Then the type will be inferred correctly after parsing the section with parseSection. You can superjson.parse() the options from the section in the parseSection call:
parseSection({
        ...section,
		options: superjson.parse(section.options),
        collapsed: collapseStates.at(0)?.collapsed ?? false,
        items: section.items.map(({ integrations: itemIntegrations, ...item }) => ({
          ...item,
          integrationIds: itemIntegrations.map((item) => item.integration.id),
          advancedOptions: superjson.parse<BoardItemAdvancedOptions>(item.advancedOptions),
          options: superjson.parse<Record<string, unknown>>(item.options),
        })),
      }),

@Meierschlumpf
Copy link
Member

I'll take a look at it tomorrow, will fix the merge conflicts and the commits that are misleading in the current state because of my force push

@jackwellerreal
Copy link
Contributor Author

@manuel-rw Thank you for reviewing the PR. Most of the changes you requested weren't commited by me, they were already commited in dev, I tried pulling the latest version of this repo so I would have the latest SQL migrations and no merge conflicts. It seems I messed something up with my fork and it decided to put all of the commits from dev into this PR. Please let me know how I should fix this. Thanks

@manuel-rw
Copy link
Member

manuel-rw commented Feb 19, 2025

@manuel-rw Thank you for reviewing the PR. Most of the changes you requested weren't commited by me, they were already commited in dev, I tried pulling the latest version of this repo so I would have the latest SQL migrations and no merge conflicts. It seems I messed something up with my fork and it decided to put all of the commits from dev into this PR. Please let me know how I should fix this. Thanks

Sorry, I didn't see that you weren't the author. You're correct, there are lots of other commits included: https://github.com/homarr-labs/homarr/pull/2334/commits
You possibly merged homarr-labs:dev into jackwellerreal:dev instead of rebasing jackwellerreal:dev onto homarr-labs:dev. This will "copy" the commits instead of re-basing your commits ontop of homarr-labs:dev, which is what we prefer.

See https://www.atlassian.com/de/git/tutorials/merging-vs-rebasing and this image:
image

I will have to check with @Meierschlumpf how we can fix this. We'll likely have to cherry pick and force push this branch. This means you'll have to delete your branch and re-pull once this is done. I'll let you know.

@jackwellerreal
Copy link
Contributor Author

Sorry for any inconveniences.

@Meierschlumpf
Copy link
Member

Okay I fixed it and removed the migrations. So you'll need to add them again 😄

@jackwellerreal
Copy link
Contributor Author

Is this good?

@Meierschlumpf
Copy link
Member

The sqlite migration is missing

@jackwellerreal
Copy link
Contributor Author

Thanks for the feedback! Will start working on requests.

@jackwellerreal
Copy link
Contributor Author

I have implemented the requested changes. It seems there has been some changes with how dynamic-actions.ts handles creating dynamic sections which requires me to change code in this PR. I could not figure out how to rebase the fork to get the latest files so I can fix the merge conflicts. Could someone please guide me on how to do that.

@Meierschlumpf
Copy link
Member

Before you do it:

  1. Remove the migrations again
  2. Change the emptySuperJSON value to {"json": {}} (with a space between : and { to not have it as part of the migration. (we might change it to superjson.stringify({}) at some point where we have to change those entities anyway

You can use

  1. git remote add upstream https://github.com/homarr-labs/homarr.git add remote for homarr-labs
  2. git fetch upstream fetch all branches from homarr-labs
  3. git checkout -b upstream_dev upstream/dev create new branch named upstream_dev from homarr-labs dev branch
  4. git switch dev switch back to your forks dev branch
  5. git merge upstream_dev merge the changes from homarr-labs into your branch

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.

This is going in a very good direction, thank you for the effort so far ❤️

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.

Great work, only 2 small things remaining 😄

@Meierschlumpf Meierschlumpf requested a review from manuel-rw March 1, 2025 13:18
@Meierschlumpf
Copy link
Member

Fix the typecheck issues and then it's okay from my side

@Meierschlumpf Meierschlumpf self-requested a review March 1, 2025 22:58
Meierschlumpf
Meierschlumpf previously approved these changes Mar 1, 2025
@Meierschlumpf
Copy link
Member

@manuel-rw can you take a quick look at this as well?

@manuel-rw
Copy link
Member

I found a bug. To reproduce:

  1. Enter edit mode
  2. Create a new dynamic section (this is important!)
  3. Edit the border color of the new section
  4. Save the board

Expected: The border color get's applied and stored in the database.
Actual: The border color get's briefly applied but not stored in the database.

The color is sent over network in the initial request, but not stored in the database:
image
image

Here's a full capture:
border-color-bug

@jackwellerreal
Copy link
Contributor Author

jackwellerreal commented Mar 2, 2025

That is really strange. Let me test it real quick

Edit: I am also getting the bug.

Copy link
Member

@manuel-rw manuel-rw left a comment

Choose a reason for hiding this comment

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

Just minor details, looks good otherwise

@jackwellerreal jackwellerreal requested a review from manuel-rw March 2, 2025 10:15
@manuel-rw manuel-rw merged commit 7dfb108 into homarr-labs:dev Mar 2, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add border customization for dynamic section
3 participants