Skip to content

feat: labels indicator #1161

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

Merged
merged 21 commits into from
Jun 4, 2024
Merged

feat: labels indicator #1161

merged 21 commits into from
Jun 4, 2024

Conversation

setchy
Copy link
Member

@setchy setchy commented May 31, 2024

Screenshot 2024-05-30 at 10 58 30 PM

@setchy setchy added the enhancement New feature or enhancement to existing functionality label May 31, 2024
@setchy setchy added this to the Release 5.7.0 milestone May 31, 2024
@setchy
Copy link
Member Author

setchy commented May 31, 2024

Tests still WIP.

Would appreciate feedback on color and icon. Went with a purple tag for the time being

@bmulholland
Copy link
Collaborator

My take is that using color where it's not conveying information makes it harder to parse the places where color is important (like PR status). Matching the text color should be fine, right?

@setchy
Copy link
Member Author

setchy commented May 31, 2024

My take is that using color where it's not conveying information makes it harder to parse the places where color is important (like PR status). Matching the text color should be fine, right?

appreciate the feedback. thanks @bmulholland. I'll make that change.

I might also try and see what render the labels as pills using the label.color looks like (like the Android GitHub app does)

@setchy setchy marked this pull request as ready for review June 2, 2024 21:00
@setchy
Copy link
Member Author

setchy commented Jun 2, 2024

updated to use pill format as to-be introduced by #1169

Screenshot 2024-06-02 at 5 18 09 PM

Copy link
Member

@afonsojramos afonsojramos left a comment

Choose a reason for hiding this comment

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

Was about to finish the merge, but had to do the dishes 😂
I think you're still missing a couple of places where labels need to be defined as I still see a couple of these around.
image

@setchy
Copy link
Member Author

setchy commented Jun 4, 2024

Was about to finish the merge, but had to do the dishes 😂 I think you're still missing a couple of places where labels need to be defined as I still see a couple of these around. image

Haha - sorry about that.

Where are you seeing this?

EDIT: found it. thanks! update coming shortly

@afonsojramos
Copy link
Member

Haha - sorry about that.

Where are you seeing this?

image On these two under `useNotifications.test.ts`

@setchy
Copy link
Member Author

setchy commented Jun 4, 2024

Updated the mocks and increased null handling just incase

Copy link
Member

@afonsojramos afonsojramos left a comment

Choose a reason for hiding this comment

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

Looking clean now, thanks 🧽

@afonsojramos afonsojramos merged commit 0752d08 into main Jun 4, 2024
6 of 7 checks passed
@afonsojramos afonsojramos deleted the feature/labels-indicator branch June 4, 2024 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants