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

Add $darken_ Utility Functions for Darkening Palette Colors #728

Merged
merged 10 commits into from
Aug 21, 2024

Conversation

shivam-daksh
Copy link
Contributor

Description

This PR introduces utility functions $darken1, $darken2, and $darken3 to darken palette colors for use in button hover states and other components. These utilities are added to the KThemePlugin and are now available globally across all Vue components.

Issue addressed

#726

Before/after screenshots

image

Details

New Utility Functions:

  • $darken1(color): Slightly darkens the provided color.
  • $darken2(color): Moderately darkens the provided color.
  • $darken3(color): Heavily darkens the provided color.

These functions are defined in a new file, darkenUtils.js, which handles the color manipulation logic.

Integration:

  • KThemePlugin.js:

    • Imported the darken utility functions from darkenUtils.js.
    • Registered these utilities on the Vue prototype as $darken1, $darken2, and $darken3, making them globally accessible.
  • Example Usage in Vue Components:

    computed: {
      styles() {
        return {
          backgroundColor: this.$darken1(this.$themePalette.red.v_1100),
        };
      }
    }

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

After review

  • The changelog item has been pasted to the CHANGELOG.md

Comments

@shivam-daksh
Copy link
Contributor Author

Hi @MisRob, Please review the PR.

@shivam-daksh shivam-daksh marked this pull request as draft August 17, 2024 07:38
@MisRob MisRob requested review from MisRob and AllanOXDi August 17, 2024 09:12
@MisRob MisRob self-assigned this Aug 17, 2024
@MisRob MisRob requested a review from ozer550 August 17, 2024 15:15
@MisRob
Copy link
Member

MisRob commented Aug 19, 2024

Thanks @shivam-daksh, the approach looks well to me overall! And very helpful PR description. I will loop in my colleagues for a full review - @ozer550 and/or @AllanOXDi, would you please have a look and also tried to test these utilities on a few colors, here from KDS or even Kolibri? The linked issue contains all the expectations. Thank you :)

@MisRob MisRob marked this pull request as ready for review August 19, 2024 13:37
@shivam-daksh shivam-daksh requested a review from MisRob August 20, 2024 10:47
@AllanOXDi
Copy link
Member

HI @shivam-daksh Thanks for your work here. I am taking look by testing it in few places

Copy link
Member

@AllanOXDi AllanOXDi left a comment

Choose a reason for hiding this comment

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

Thanks @shivam-daksh , LGTM!

Copy link
Contributor Author

@shivam-daksh shivam-daksh left a comment

Choose a reason for hiding this comment

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

looks fine

package.json Show resolved Hide resolved
@shivam-daksh
Copy link
Contributor Author

Hey, @AllanOXDi and @MisRob , There was a issue persisting check if changelog.md is changed , So I tried to update the changes in CHANGELOG.md manually. I thought it may disturb the workflows, so I reverted the changes. Waiting for your guidance and approval.

@shivam-daksh shivam-daksh requested a review from AllanOXDi August 21, 2024 04:23
@AllanOXDi
Copy link
Member

AllanOXDi commented Aug 21, 2024

Hi @shivam-daksh All you need is to add this to the changelog. Something like

 [#728]
  - **Description:** [add the description of your PR  here]
  - **Products impact:**  None 
  - **Addresses:** https://github.com/learningequality/kolibri-design-system/issues/726
  - **Components:**  
  - **Breaking:** N0
  - **Impacts a11y:** No
  - **Guidance:**
  [#728]https://github.com/learningequality/kolibri-design-system/pull/728

@shivam-daksh
Copy link
Contributor Author

Hey @AllanOXDi , I've changed the changelog file. Please review this again.

@AllanOXDi
Copy link
Member

AllanOXDi commented Aug 21, 2024

Thanks! look better. But you still need to add the description. Here is an example https://github.com/learningequality/kolibri-design-system/pull/722/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR8. I am sorry I was not clear.
If you could also resolve the conflicts- would be good

@shivam-daksh
Copy link
Contributor Author

@AllanOXDi , Do I've to change other files in documentation as given in this example or I've to change just CHANGELOG.md?

@AllanOXDi
Copy link
Member

@AllanOXDi , Do I've to change other files in documentation as given in this example or I've to change just CHANGELOG.md?

No! What you did was good. just supposed to add the description of your work under the description

@AllanOXDi
Copy link
Member

Looks good! lets now just resolve the merge conflicts

@shivam-daksh
Copy link
Contributor Author

Looks good! lets now just resolve the merge conflicts

@AllanOXDi, It's done.

@AllanOXDi
Copy link
Member

Looks good! lets now just resolve the merge conflicts

@AllanOXDi, It's done.

Thanks!

@MisRob
Copy link
Member

MisRob commented Aug 21, 2024

Thanks both @shivam-daksh and @AllanOXDi, and @shivam-daksh thanks for you first contribution. We appreciate it. I will merge soon.

FYI, I made use of one of my better screens and tweaked the values to correspond a bit more closely to resulting hex values mentioned in the issue.

Important part of acceptance criteria is documentation, however we typically do not require volunteers to write documentation, so I jotted it down and am pushing that as well:

Screenshot from 2024-08-21 15-33-03

@MisRob MisRob mentioned this pull request Aug 21, 2024
@MisRob
Copy link
Member

MisRob commented Aug 21, 2024

Opened related issue #740

@MisRob MisRob merged commit 563e831 into learningequality:develop Aug 21, 2024
8 checks passed
@MisRob
Copy link
Member

MisRob commented Aug 21, 2024

@shivam-daksh if you're still interested in the follow-up issues

now it's the right time :)

Just message us in those issues.

@shivam-daksh
Copy link
Contributor Author

Hi @MisRob, Thanks a lot for the feedback and tweaks! I appreciate you taking the time to document the darken utilities as well. It’s been a great learning experience. Although it was my second contribution (my first contribution was Links leading outside Studio need to have a pop out icon. I would love to working on those issues.

@MisRob
Copy link
Member

MisRob commented Aug 21, 2024

@shivam-daksh Lovely! You will need to post comment there, otherwise we can't assign. Get some rest too ;)

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