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

[#147] Updated Chip component color mapping. #176

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

govindmaloo
Copy link
Collaborator

@govindmaloo govindmaloo commented Jun 2, 2024

Checklist before requesting a review

  • I have formatted the subject to include the issue number
    as [#123] Verb in past tense with a period at the end.
  • I have provided information in the Changed section about WHY something was
    done if this was a bespoke implementation.
  • I have performed a self-review of my code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have run new and existing relevant tests locally with my changes,
    and they have passed.
  • I have provided screenshots, where applicable.

#147

Changed

  1. Updated Chip component colour mapping.

Screenshots

Hover:
Screenshot 2024-06-02 at 10 19 01 PM

Selected:
Screenshot 2024-06-02 at 10 19 09 PM

Selected:hover:
Screenshot 2024-06-02 at 10 19 18 PM

Dark
Screenshot 2024-06-02 at 10 18 41 PM

Hover:
Screenshot 2024-06-02 at 10 18 26 PM

Selected:
Screenshot 2024-06-02 at 10 18 07 PM

Selected:hover:
Screenshot 2024-06-02 at 10 18 15 PM

@govindmaloo govindmaloo self-assigned this Jun 2, 2024
@govindmaloo govindmaloo requested a review from AlexSkrypnyk June 2, 2024 16:51
Copy link

github-actions bot commented Jun 2, 2024

@github-actions github-actions bot temporarily deployed to pull request June 2, 2024 16:54 Inactive

&.ct-theme-light {
&.active {
&:hover {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this to the map above as active-hover and update ct-button-type() mixin to support active-hover type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@AlexSkrypnyk I have updated the Pr as per your suggestions.

@AlexSkrypnyk AlexSkrypnyk added the PR: Requires more work Pull request was reviewed and reviver(s) asked to work further on the pull request label Jun 2, 2024
@github-actions github-actions bot temporarily deployed to pull request June 3, 2024 16:58 Inactive
@AlexSkrypnyk AlexSkrypnyk merged commit 78c789f into main Jun 3, 2024
5 checks passed
@AlexSkrypnyk AlexSkrypnyk deleted the feature/147-chip-color-mapping branch June 3, 2024 20:40
@sonamchaturvedi28
Copy link

Test Env: DEV
Test Status: FAIL
Test Result:

  • Chip light and dark themes - Default, Hover and Selected states : PASS
  • Chip light and dark themes - Selected hover state : only active state styles are not getting applied. FAIL
Screenshot 2024-06-05 at 12 05 40 PM

@govindmaloo
Copy link
Collaborator Author

@sonamchaturvedi28

Updated styles are for selected and selected hover.

Storybook have knob that allow you to make the chip selected.

I am not sure active state applied for chip.

@AlexSkrypnyk
Copy link
Contributor

Atoms___Chip_-_Chip_⋅_Storybook Cursor_and_Atoms___Chip_-_Chip_⋅_Storybook

Checked the colours - work as expected. PASS

@fionamorrison23 fionamorrison23 added this to the 1.8 milestone Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Requires more work Pull request was reviewed and reviver(s) asked to work further on the pull request
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

4 participants