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

Upgrade the unstable branch to KDS v3.0.0 #4412

Merged
merged 3 commits into from
Jan 31, 2024

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Jan 22, 2024

Summary

Description of the change(s) you made

Upgrades KDS version in Studio from the KDS commit e9a2ff34716bb6412fe99f835ded5b17345bab94 from Jun 7, 2022, to the new KDS v3.0.0 (and several versions in between).

Manual verification steps performed

First, I audited Studio, KDS release notes and diffs to get an idea of the scope and affected places. Results are here #4411.

For components that shouldn't have breaking updates, I previewed them visually at a few places. Quite often on both desktop and mobile. I also tried to use them when relevant. Checked the nature of changes in the release notes, and if I knew about some updates that weren’t in the older release notes, I peaked into git diffs to see if there’s something to be careful about.

I also tried to observe unexpected UI shifts and related browser errors when navigating the Studio. Haven’t noticed anything but I likely didn’t visit all pages.

For components with breaking updates:

  • KCheckbox: Made an update, see the commit messages.
  • KButton: Searched for buttons that use both text prop and default slot since buttons are said to break only in this case. I haven't find any.
    KCheckbox: Searched for checkboxes that have <label> in their default slot since the breaking change applies only to them. I haven't find any.

Also clicked all these and tested some workflows that use them.

Reviewer guidance

First, please read through #4411 that includes Studio audit and linked KDS release notes. Then, you could

  • Check if code changes make sense. See commit messages where I explained why the update was needed. You can find more information in the KDS release notes if needed.
  • Is something missing from the audit in #4411?
  • Are there some places that I should update but didn't update?
  • Are components affected still working well?
  • Can you see some errors in the browser error?
  • Can you see some unexpected UI problems?
  • Can you think of anything else?

Are there any risky areas that deserve extra testing?

  • i18n, LTR
  • Buttons
  • Checkboxes
  • Radio buttons (storage request form and language switcher modal)
  • Modals (no "itentionally breaking" updates but went through a significant refactor of some internals)
  • Circular loaders
  • Grids
  • Icons
  • Textboxes

References

Contributor's Checklist

PR process:

  • If this is an important user-facing change, PR or related issue the CHANGELOG label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later time
  • If this includes an internal dependency change, a link to the diff is provided
  • The docs label has been added if this introduces a change that needs to be updated in the user docs?
  • If any Python requirements have changed, the updated requirements.txt files also included in this PR
  • Opportunities for using Google Analytics here are noted
  • Migrations are safe for a large db

Studio-specifc:

  • All user-facing strings are translated properly
  • The notranslate class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)
  • All UI components are LTR and RTL compliant
  • Views are organized into pages, components, and layouts directories as described in the docs
  • Users' storage used is recalculated properly on any changes to main tree files
  • If there new ways this uses user data that needs to be factored into our Privacy Policy, it has been noted.

Testing:

  • Code is clean and well-commented
  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Any new interactions have been added to the QA Sheet
  • Critical and brittle code paths are covered by unit tests

Reviewer's Checklist

This section is for reviewers to fill out.

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

Installs the dependency
Fixes "Error: Can't resolve 'kolibri-design-system/lib/utils/i18n'"
caused by removal of this file in KDS v2.0.0 by copying the removed
file content directly to Studio.
Remove deprecated `value` prop in favor of the new
`buttonValue` prop that's supposed to replace it
in `KRadioButton`.
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.

Hi Misha! I went through all components that have changed from e9a2ff34716bb6412fe99f835ded5b17345bab94 to v3.0.0 that were present in studio and didn't find any errors either in the console or visually.

But I noticed that the accessibility issue about the invisible outline in the KCheckbox and KRadioButtons when navigating with the keyboard is still present after the KDS update. I saw a bit about why it was happening and it seems to be an issue with the globalThemeState.inputModality not recognizing that we are navigating with the keyboard, but it's definitely not something related to the KDS update as it was happening before too, so I'll open a new issue for this.

@bjester
Copy link
Member

bjester commented Jan 26, 2024

Seems the radio buttons in the storage request form aren't working anymore? I click and nothing happens
image

@AlexVelezLl
Copy link
Member

Oh I also checked that place @bjester and didn't find any errors. But I just replicated it now, it seems that it is necessary to delete the node_modules and do yarn install again, I did it when I reviewed it just in case. It seems like yarn is not refreshing the KDS package properly. It can maybe be because in the package.json of KDS we havent updated the "version" field? It is still "v1.3.0" and maybe yarn doesnt update the KDS package in the node_modules because of that.

@bjester
Copy link
Member

bjester commented Jan 26, 2024

@AlexVelezLl Oh that's a good catch! Any reason we aren't updating the version in KDS package.json? Alex's hunch may be correct. (Also I don't think the version in there should start with v, e.g. https://github.com/lodash/lodash/blob/main/package.json#L3)

@rtibbles
Copy link
Member

I had flagged this to @MisRob earlier this week too, so I think it will be going into the checklist for future releases (although I had also missed the preceding v issue!), so hopefully this will not be a problem for subsequent upgrades!

@MisRob
Copy link
Member Author

MisRob commented Jan 30, 2024

Yes, apologies, I forgot to do that. Thanks @AlexVelezLl, as you say, removing node_modules should resolve the issue @bjester observed.

@marcellamaki marcellamaki self-assigned this Jan 31, 2024
@bjester
Copy link
Member

bjester commented Jan 31, 2024

Yes, apologies, I forgot to do that. Thanks @AlexVelezLl, as you say, removing node_modules should resolve the issue @bjester observed.

Requiring any and all contributors to delete KDS from node_modules isn't the easiest thing to raise awareness too, especially when it's temporary. If the only potential issue someone might run into is a broken storage request form, I'm fine with merging this.

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

Great - thanks @bjester and @AlexVelezLl for your reviews, and @MisRob for your work on this! The code changes make sense to me as well, so let's go ahead and merge. I think anything that might come up during later QA would be small enough that we can address separately.

And thanks, @AlexVelezLl for noticing the focus problem with the inputModality and offering to write a follow up issue for it! that would be great. :)

@marcellamaki marcellamaki merged commit 6bd81d4 into learningequality:unstable Jan 31, 2024
11 checks passed
@bjester bjester mentioned this pull request Feb 23, 2024
24 tasks
@akolson akolson mentioned this pull request Aug 13, 2024
@akolson akolson mentioned this pull request Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade the unstable branch to KDS v3.0.0 Install KDS Version 2.0.0
5 participants