-
-
Notifications
You must be signed in to change notification settings - Fork 288
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
Add tag on scene #1900
Add tag on scene #1900
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1900 +/- ##
=======================================
Coverage 98.17% 98.17%
=======================================
Files 773 776 +3
Lines 12133 12167 +34
=======================================
+ Hits 11911 11945 +34
Misses 222 222 ☔ View full report in Codecov by Sentry. |
Job #2043: Bundle Size — 9.01MiB (+0.24%).
Warning Bundle contains 3 duplicate packages – View duplicate packages Bundle metrics
Bundle size by type
View job #2043 report View callemand:AddTagOnScene branch activity |
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.
Thanks for the PR ! It works great 👏
I have a few feedbacks, the major one is on the DB side.
I really think we should stay relational in Gladys and store this as a relation table :
t_tag_scene:
- scene_id uuid
- name text
It'll let you find existing tags pretty easily so you can display the existing tags in scene (otherwise, right now, the user can easily create duplicate tags)
On the UX, we should have a way to filter easily on the list of scene by tags (here again, you'll need a query to get all tags -> only possible in an efficient way with a relation table)
The general new UX of scene is really great and IMO users will love it !
Thanks for looking into this 😄
9a1626f
to
9a13fbd
Compare
I've try to implements the 't_tag_scene'. it's working but. Do I need to create a new api controller to get the tags ? |
@callemand You can do it in the "scene" controller :) |
e9bc087
to
7ce7ad6
Compare
9aa839f
to
d5e726a
Compare
26902e3
to
8784df6
Compare
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.
Thanks for this PR ! The UI is really great like that 👏
A few feedbacks on the UI side:
- When I click outside of the tag select dropdown, it could be great to close the dropdown
- The current behavior when I select multiple tags is to do a "AND", and when comparing with other software, I have the feeling that usually it's more a "OR" done on that. What do you think of switching to OR ?
Kapture.2023-11-06.at.10.05.22.mp4
I was able to create a super long tag and then it break the UI :D
Could we limit the tag length ?
I have a few comments on the backend side (see comments in code)
Thanks for this PR 🙌
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.
Thanks for the changes, it's way better! 👏
A few feedbacks:
When I click on a tag, it's a bit weird I can only click on the "title" but on the full width so when titles are a bit longer it's weird to have to change where to click:
Kapture.2023-11-10.at.14.13.28.mp4
It seems were are looking "scene density" on the scene list.
Before:
After:
What about going back to 4 per width?
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.
Thanks for the fixes!
For the click on the tags, it seems it's the same as before?
Kapture.2023-11-13.at.10.58.08.mp4
For the responsive, maybe on mobile we should hide the "Nouveau" text and just keep the "+" (like we do on some pages) ?
And I'm wondering if we don't need a button "unselect all" at the top ?
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.
3198edd
to
bb6be08
Compare
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.
Good for me now! 👏
Pull Request check-list
To ensure your Pull Request can be accepted as fast as possible, make sure to review and check all of these items:
npm test
on both front/server)npm run eslint
on both front/server)npm run prettier
on both front/server)npm run compare-translations
on front)NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.
Description of change