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

feat: add appearanceOverrides prop to KImg #509

Merged
merged 5 commits into from
Jan 15, 2024

Conversation

EshaanAgg
Copy link
Contributor

@EshaanAgg EshaanAgg commented Dec 20, 2023

Description

Adds the appearanceOverrides prop to KImg component for styling

Issue addressed

Fixes #371

Changelog

  • #509
    • Description: Introduces appearanceOverrides prop for the KImg component
    • Products impact: new API
    • Addresses: KImg: Add 'appearanceOverrides' #371
    • Components: KImg
    • Breaking: No
    • Impacts a11y: Yes
    • Guidance: Adds a way to style the component intrinsically, allowing users to make more a11y compatible components

At a high level, how did you implement this?

I added a new prop to the component and then passed its styles to the styleObject.

Does this introduce any tech-debt items?

No

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

@MisRob
Copy link
Member

MisRob commented Jan 9, 2024

Thanks @EshaanAgg, and apologies for delayed response. I've just returned from my vacation and the whole team had holidays too. I will have a look in a few days.

@MisRob MisRob self-requested a review January 9, 2024 14:03
@MisRob
Copy link
Member

MisRob commented Jan 15, 2024

Thank you @EshaanAgg, all is looking great. Thank you for taking care of the documentation page. I will just tweak the changelog a bit, resolve the changelog conflict and then merge.

- **Components:** KImg
- **Breaking:** No
- **Impacts a11y:** No
- **Guidance:** -
Copy link
Member

@MisRob MisRob Jan 15, 2024

Choose a reason for hiding this comment

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

Thank you for being diligent and providing guidance part @EshaanAgg. I removed it in my before merge commit because we rather want to discourage using this prop and it doesn't affect a11y directly as much as was indicated in your comment. Its main purpose is to allow flexible updates of styles when the implemented API is not sufficient, but overall we don't want to rely on it much.

@MisRob MisRob merged commit 3cb2b83 into learningequality:main Jan 15, 2024
8 checks passed
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.

KImg: Add 'appearanceOverrides'
2 participants