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

Assign custom fields to categories #930

Merged
merged 15 commits into from
Apr 30, 2024
Merged

Conversation

rockingrohit9639
Copy link
Contributor

Fix - #658

Copy link
Contributor

@DonKoko DonKoko left a comment

Choose a reason for hiding this comment

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

Okey considering this is the first part of the scope, its great. I tested it and I couldn't find any issues myself.
The next step is managing it on the asset side, so a few things:

  • 1. When creating/editing an asset, we should show fields only for custom fields whos categories include the current category of the asset. If custom field doesnt have any categories, we show it on all assets.
  • 2. For a better UX i think we need to make this dynamic, so without needing to save the asset. So lets say you are creating a new asset, once you select a category, we should update the UI and show the fields related to that category.
  • 3. Once the values are saved they should stay in the DB even if the asset category changes. So if an asset has category A and some custom fields values saved, and then then asset category gets changed to B, we should not remove those CustomFieldValues from the db. They should stay there so if in the future you switch back to category A they are present.
  • 4. This means we also have to update the asset page to display only values from the current category(basically the same as create/edit)
  • 5. On the assets backup export, we need to make sure all values are exported, even the ones that wont be displayed currently due to the asset category. I think this should work already, but we have to test it.
  • 6. On the custom fields index, lets add a column (before required) called "Categories" and display a list of the categories names separated by commas. If there are none we can just show "All"

I honestly didn't realize there are no designs provided for this, but I think it should be fine. Its mostly logic. Only new UI element is the column on the custom fields table but that shouldn't be an issue I think.

@DonKoko
Copy link
Contributor

DonKoko commented Apr 26, 2024

@rockingrohit9639 1 thing I thought of. For the part of the scope where we display the categories on the custom fields index. Can you please make it work the same way we display tags on the asset index? It basically shows 2 and then a small icon with a title with the rest of the tags. You can check it in ListItemTagsColumn

@rockingrohit9639
Copy link
Contributor Author

Ok @DonKoko
Got it 👍

@carlosvirreira
Copy link
Contributor

Amazing @rockingrohit9639 !

Copy link
Contributor

@DonKoko DonKoko left a comment

Choose a reason for hiding this comment

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

A few things I found that need fixing:

  • you can add the same category 2 times to a custom field. Should not be possible. This will actually be automatically fixed when you save but we should still not allow it in the frontend Screenshot 2024-04-29 at 10 25 37.
  • the button to clear a category next to the select is bigger in size. lets make them consistent.
  • We have a new relationship table called _CategoryToCustomField. This is automatically created by prisma. By default tables on supabase have RLS disabled. Can you please create a new emtpy migration to enable RLS for this relationship table. You can look at old migration for example: https://github.com/Shelf-nu/shelf.nu/blob/main/app/database/migrations/20230428080057_add_rls_to_qr/migration.sql

app/modules/asset/service.server.ts Show resolved Hide resolved
DonKoko
DonKoko previously approved these changes Apr 29, 2024
@jurrejansen
Copy link

@DonKoko
Copy link
Contributor

DonKoko commented Apr 29, 2024

@jurrejansen I actually think it is related. @rockingrohit9639 can you check it. I remember you mentioned some issue with the defaultValue. Maybe its related

@carlosvirreira
Copy link
Contributor

A custom field toggle can be set to TRUE - if you do not select a category (you click on the 'close' cross) then you can simply save the page. This will throw no errors.

However, if you then refresh the page the toggle goes back to 'False'.

Could cause confusions - not a breaking issue.


@DonKoko DonKoko merged commit 2bb686f into Shelf-nu:main Apr 30, 2024
4 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.

None yet

4 participants