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

Feature/search tag #41

Merged
merged 2 commits into from
Apr 12, 2024
Merged

Feature/search tag #41

merged 2 commits into from
Apr 12, 2024

Conversation

srsingh04
Copy link
Contributor

@srsingh04 srsingh04 commented Apr 6, 2024

This is the code for the UI of the tags. The tags will be rather personalized for each users depending on what activities they do.

@ntissieres
Copy link
Contributor

I think we need to discuss more in details how we will implement the tag hierarchy, and follow it for the implementation of this feature. Like if there is more than a number of tags from the same category, we could group them and display the parent tag. When the user click on this parent tag, the children would appear. I will clarify it with you tomorrow if it's not clear.

Copy link
Contributor

@ntissieres ntissieres left a comment

Choose a reason for hiding this comment

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

Maybe it's not a good idea to name the file Tags.kt, we already have a file named Tag to define the data class and moreover it's not truly indicative of what it encodes. MainscreenTagsFilter would be better for example

Copy link
Contributor

@ntissieres ntissieres left a comment

Choose a reason for hiding this comment

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

Don't forget to remove everything that is commented. In order to merge the code into the main, the code should be clean

Copy link
Contributor

@ntissieres ntissieres left a comment

Choose a reason for hiding this comment

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

The preview doesn't work because of the TagScreen which is not define. It's easier to review the code if we have a visualisation of the feature, if some modifications concerning the UI is necessary

Copy link
Contributor

@ntissieres ntissieres left a comment

Choose a reason for hiding this comment

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

It's better to remove the padding on the top of the items, it will be done after when we add the search tag to the scaffold

Copy link
Contributor

@ntissieres ntissieres left a comment

Choose a reason for hiding this comment

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

Perfect !

Copy link

@ntissieres ntissieres merged commit ae4d4dc into main Apr 12, 2024
2 checks passed
@ntissieres ntissieres deleted the feature/SearchTag branch April 12, 2024 13:21
@violoncelloCH violoncelloCH added this to the Milestone 1 milestone Apr 19, 2024
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.

3 participants