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

/deep/ to ::v-deep #504

Merged
merged 2 commits into from
Dec 10, 2024
Merged

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Dec 8, 2023

Description

  • Removes two unused /deep/ selectors in KSelect that were referencing non-existent classes
  • Replaces the two remaining /deep/ selectors used in KDS with ::v-deep https://vue-loader.vuejs.org/guide/scoped-css.html#deep-selectors
  • This behaves identically, but has the advantage of not breaking future versions of Sass (i.e. Dart Sass instead of Node Sass, which is deprecated)

Issue addressed

This is required as part of an extended yak shave in order to ultimately upgrade to newer NodeJS versions, where building Node Sass becomes increasingly more challenging.

Changelog

  • Description: Remove use of /deep/ in favour of ::v-deep
  • Products impact: none
  • Addresses: -
  • Components: -
  • Breaking: no
  • Impacts a11y: -
  • Guidance: -

Steps to test

  1. Ensure that KSelect still functions as intended when disabled.
  2. Ensure that the date input styling in the docs page for the date picker is still correct
  3. Ensure that the Icon is still blue on the icons page showing what not to do

(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

@rtibbles rtibbles added the TODO: needs review Waiting for review label Dec 8, 2023
@rtibbles rtibbles marked this pull request as draft December 8, 2023 20:52
@rtibbles
Copy link
Member Author

rtibbles commented Dec 8, 2023

Converting to draft - I can't make this comply with linting without fixing kolibri linting tools, it seems. Yak shave continues!

@MisRob
Copy link
Member

MisRob commented Dec 12, 2023

Okay, let us know when the time comes ;)

@rtibbles rtibbles marked this pull request as ready for review December 10, 2024 17:03
Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

lgtm!

@AlexVelezLl AlexVelezLl merged commit 6ee245a into learningequality:develop Dec 10, 2024
9 of 11 checks passed
learning-equality-bot bot pushed a commit that referenced this pull request Dec 10, 2024
@rtibbles rtibbles deleted the thatsdeepman branch January 15, 2025 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants