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

Remove KResponsiveElementMixin #783

Conversation

AlexVelezLl
Copy link
Member

Description

Make useKResponsiveElement public and remove KResponsiveElementMixin.

Issue addressed

Addresses #711.

Changelog

Steps to test

  1. Look for any reference to KResponsiveElementMixin in the lib or in the docs.
  2. Do KBreadcrumbs or KFixedGrid have any regressions?

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

Comments

@AlexVelezLl AlexVelezLl requested review from MisRob, nucleogenesis and AllanOXDi and removed request for MisRob September 16, 2024 12:56
@MisRob
Copy link
Member

MisRob commented Sep 23, 2024

Hi @AlexVelezLl, I'm getting Page Not Found when trying to access the new documentation page https://deploy-preview-783--kolibri-design-system.netlify.app/usekresponsiveelement, any idea why?

@AlexVelezLl
Copy link
Member Author

Thank you for noticing the not found page @MisRob! It was beacuse I was using a wrong doc file name. I just updated this and its solved now!

@AlexVelezLl AlexVelezLl requested a review from MisRob September 23, 2024 15:37
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

The code changes look good to me.

One thing that came to mind is that maybe we should deprecate KResponsiveElementMixin rather than just removing it outright. This would give us time to make the requisite changes in Kolibri.

Perhaps we could keep it and put a consol.warn() in it's created() hook saying "KResponsiveElement has been deprecated and will be removed in the next release. Please use useKResponsiveElement".

In any case, once this merged can you make an issue in Kolibri for making the necessary changes there

@AlexVelezLl
Copy link
Member Author

Thank you @nucleogenesis! Yes, the ideal case is deprecate it before we remove it, but we discussed this with Richard and we wanted to make the removal part of the breaking changes of KDS v5.

As per Richard's comment talking:

Yes, I think that would be good (especially as we have already explicitly deprecated the KResponsiveWindowMixin - not sure we did an explicit deprecation warning for KResponseElementMixin, but it's used significantly less - I think only once in Kolibri).

And yes, there is already an issue in Kolibri that is blocked by this PR learningequality/kolibri#12523, it is currently marked as help wanted, but I can take care of this and update the dependanbot PR updating the kds version as is a very straightforward change.

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Everything looks good to me w/ that clarification @AlexVelezLl thank you!

@AlexVelezLl AlexVelezLl merged commit 2a7f17d into learningequality:develop Sep 30, 2024
8 checks passed
@AlexVelezLl AlexVelezLl deleted the remove-kresponsive-element-mixin branch September 30, 2024 22:01
learning-equality-bot bot pushed a commit that referenced this pull request Sep 30, 2024
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.

Make _useKResponsiveElement composable public and Remove KResponsiveElementMixin
3 participants