-
Notifications
You must be signed in to change notification settings - Fork 0
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
Created Dialog for Adding Tags #74
Conversation
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.
Looks good, Just wondering if we can consolidate chip setup so we can get all the styling to look the same and reduce duplicated code
// Check if the tag is not empty | ||
if (!tagText.isEmpty()) { | ||
// Create a new Chip and set its properties | ||
Chip chip = new Chip(requireContext()); |
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.
Not sure if its viable/feasible but could we consolidate the keyword chip and tag chip into some util method like setupChip(chipText, chipGroup);
? Look at autoFillFilter() in KeywordFilterFragment.
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.
@ldbonkowski See latest commit, I applied the util to TagFragment and KeywordFilterFragment
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.
Do we not want to duplicate the colour lines too?
chip.setChipBackgroundColorResource(R.color.white);
chip.setChipStrokeColorResource(R.color.brown);
chip.setTextColor(ContextCompat.getColor(context , R.color.brown));
Also I was thinking we could call it newChip and put line 68 inside as well and just pass the context in.
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.
@ldbonkowski Addressed these in the most recent commit. I think it's as consolidated as we can get for now but lmk what you think. I do see areas that we could potentially consolidate in the future once we have selecting and applying tag functionality so we can put a pin in it until we have that feature.
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.
LGTM!
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.
I like my chips with the dip 🥔
Describe your changes
This PR accomplishes the following:
Note for reviewers:
Selecting and applying tags to items will be addressed in [US 03.03.01] Select and apply tags to items. This ticket is for defining the tags and creating the dialog.
Issue ticket number and link
This PR addresses: [US 03.01.01] Create tags to categorize items
Checklist before requesting a review
Screenshots (if applicable)