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

[Accounts] Use KDS Buttons #3891

Merged
merged 2 commits into from
Apr 18, 2023

Conversation

AlexVelezLl
Copy link
Member

@AlexVelezLl AlexVelezLl commented Dec 19, 2022

Summary

Description of the change(s) you made

Use KDS KButton or KRouterLink components instead of Vuetify's VBtn in accounts app.

Manual verification steps performed

  1. Go through the following workflows as described in Use KDS for buttons in accounts application #3148:
  • Sign-in
  • Registration
  • Password reset
  • Account deletion
  1. Check that all buttons are consistent with the Kolibri Design System.
  2. Check that all buttons work correctly.

Screenshots

A couple of remarkable changes are the replacement of raised buttons for link buttons in favor of consistency with other sections (We could keep the raised buttons if it makes more sense):

Before After
image image
image image

References

Addresses #3148

Comments

A difference between using VBtn with KButton, is that in VBtn you can specify a large button, which is not in KButton, so the buttons will look a bit smaller:

Before After
image image

Another difference is that KButton does not support any prop to make the element Block instead of inline-block, so if we want the button to fill the full width we need to add classes to set the width: 100%.

Pitfalls of replacing ActionLink with KDS components:

  • Something we have with the ActionLink component is that by default it has the :title=“text” attribute in the span element which is not in the KRouterLink or KExternalLink component (which for example enables a native browser popup in Chrome), although we could pass the span with that attribute as slot if necessary.
  • Also, KRouterLink does not support the openInNewTab functionality, something we can have with the ActionLink component in conjunction with the :to prop (In KExternalLink it is supported but not in KRouterLink).
  • The ActionLink props that control the truncate and notranslate classes are not supported in either KRouterLink or KExternalLink, although they can be achieved by passing the span as slot.

Contributor's Checklist

Studio-specifc:

  • 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

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

@AlexVelezLl AlexVelezLl force-pushed the accounts-kds-buttons branch from 401dd6b to 295bde7 Compare March 19, 2023 14:53
@bjester bjester requested a review from MisRob April 11, 2023 15:51
</VBtn>
<KButton
primary
class="w-100"
Copy link
Member

Choose a reason for hiding this comment

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

Definitely nothing blocking and can also be a matter of personal style - if that'd be helpful in future work, when there is a one-line style, it may be simpler to use and read :style="{ width: '100%' }" rather than defining a new class which is quite commonly used syntax in LE's codebases.

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.

Thank you @AlexVelezLl, all is looking good to me and I also manually tested that I can use updated links and buttons as expected. Also thanks for the helpful description of the compatibility of the previous and new approach, it helped me to understand the decisions you made in the code.

@bjester bjester merged commit 50b182f into learningequality:unstable Apr 18, 2023
@bjester
Copy link
Member

bjester commented Apr 18, 2023

Thanks for reviewing this @MisRob !

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.

3 participants