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 KSelect to KDS. #355

Merged
merged 3 commits into from
Aug 30, 2022

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Aug 25, 2022

Description

  • Does a minimally modifying port of the KSelect from Kolibri to KDS
  • This is almost completely a copy paste + adding to the KThemePlugin, however, some small changes were needed:
    • Import paths from KDS were changed to local imports
    • KSelect used the commonCoreStrings mixin in Kolibri, so this is substituted by having a prop to provide text for the clear action.
    • While doing the above, I noticed raw unwrapped text in the component - I replaced this with 'noResultsText` but did not expose it on the KSelect component, because it is currently not used anywhere.

There is almost certainly some refactor that can be done here to avoid the extra component nesting that still exists here, but that seemed better to defer to later for ease of review.

Issue addressed

Fixes #339

Before/after screenshots

Linked KSelect seems to function as before:
image

image

Steps to test

Link to Kolibri PR learningequality/kolibri#9667

Check instances of KSelect.

The clearable/clearText props are only used in the resource search panel

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 has been added to the changelog

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

Post-merge updates and tracking

  • Learning Platform update PR is submitted
  • Learning Platform update PR is merged
  • Studio update PR is submitted
  • Studio update PR is merged
  • Data Portal update PR is submitted
  • Data Portal update PR is merged

Comments

@MisRob
Copy link
Member

MisRob commented Aug 29, 2022

Thank you @rtibbles. Before I proceed to the code review, could you update the changelog and documentation? Regarding documentation, you don't need to write an elaborate page but at least having a page with a basic summary to make sure everyone knows that this component is available in KDS and what's its basic API (the API part of the page will get automatically generated from JSDoc comments)

@MisRob
Copy link
Member

MisRob commented Aug 29, 2022

Here's the preview of the docs associated with this PR https://deploy-preview-355--kolibri-design-system.netlify.app/

KSelect should appear before KSwitch

Screenshot from 2022-08-29 14-00-15

@MisRob
Copy link
Member

MisRob commented Aug 29, 2022

The "Overview" section can be brief and would ideally contain the component visualized, for example like here https://deploy-preview-355--kolibri-design-system.netlify.app/krouterlink/#overview

MisRob
MisRob previously requested changes Aug 29, 2022
Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

^ commented above

@MisRob
Copy link
Member

MisRob commented Aug 29, 2022

And regarding the changelog, if you rebase on top of the latest develop, there's already a section for develop changes that won't go into 1.4.x https://github.com/learningequality/kolibri-design-system/blob/develop/CHANGELOG.md#develop-version-not-yet-known.

@rtibbles
Copy link
Member Author

Before I proceed to the code review, could you update the changelog and documentation? Regarding documentation, you don't need to write an elaborate page but at least having a page with a basic summary to make sure everyone knows that this component is available in KDS and what's its basic API (the API part of the page will get automatically generated from JSDoc comments)

I did try to add this in the initial PR but it broke the docs build, I'll reinstate what I had previously, and perhaps you can advise.

@rtibbles rtibbles force-pushed the kds_took_my_select_away branch from fbed7a8 to 5338ba1 Compare August 29, 2022 16:25
@rtibbles
Copy link
Member Author

rtibbles commented Aug 29, 2022

Updated both - it seems the docs issue I was having was because the docs build seemed not to provide a required prop to the component at one point, I assume when doing introspection for JSDocs?

@rtibbles rtibbles force-pushed the kds_took_my_select_away branch from 5338ba1 to 53c878b Compare August 29, 2022 16:28
@rtibbles rtibbles force-pushed the kds_took_my_select_away branch from 53c878b to 325262b Compare August 29, 2022 18:25
@rtibbles
Copy link
Member Author

PR updated with documentation text authored by @jtamiace.

@nucleogenesis nucleogenesis requested review from jtamiace and removed request for jtamiace August 30, 2022 14:34
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.

Code successfully migrated, docs look great 🎉 and now finally KDS shall be whole again

@MisRob MisRob dismissed their stale review August 30, 2022 15:56

Documentation and changelog has been updated, thank you

@rtibbles rtibbles merged commit 77c5d4b into learningequality:develop Aug 30, 2022
@rtibbles rtibbles deleted the kds_took_my_select_away branch August 30, 2022 22:47
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.

Move KSelect to KDS
3 participants