[Remove Vuetify from Studio] Informative pages in Accounts #5635
Conversation
|
👋 Thanks for contributing! We will assign a reviewer within the next two weeks. In the meantime, please ensure that:
We'll be in touch! 😊 |
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
This is an excellent start - thank you!
- 8 of 9 page migrations look correct
- Screenshots provided via Google Drive link - very helpful for review (be sure to update them as you make follow up edits too).
Blocking issues:
- No tests added.
- Invalid attribute syntax for appearance.
- Incorrect import path for one migrated component.
I look forward to your updates!
| <KRouterLink | ||
| :to="{ name: 'Main' }" | ||
| :text="$tr('backToLogin')" | ||
| :appearance="basic - link" |
There was a problem hiding this comment.
blocking: Invalid attribute syntax - :appearance="basic - link" will evaluate as JavaScript (undefined - undefined = NaN), not as a string.
Should be:
appearance="basic-link"(No colon, plain string attribute)
There was a problem hiding this comment.
Thanks @rtibbles, I understood what you mentioned. I was facing an issue where binding the value as a string causes the link to appear covered with the link color. I tried removing the appearance prop since basic-link is the default, but the issue still there. Interestingly, when I bind it using JavaScript (sorry my mistake it was undefined here) the problem does not occur.
This has been confusing, as the issue only appears when using StudioMessageLayout . I will continue working on this to find the fix. I would also appreciate your insights on this
The issue:
There was a problem hiding this comment.
Interesting - I'm not seeing anything in the styles for your new component that would cause this - if you inspect the link itself, and look at the CSS inspector, are you able to see where this background colour is coming from?
There was a problem hiding this comment.
Hey @rtibbles , After spending some time debugging this is the conclusion that I came to regarding the issue.
Why this happens:
At its core, KRouterLink seems to be built on top of the button system (mixins: [buttonMixin]). Even when we use appearance="basic-link", it still creates a button that looks like a link. Further investigation in buttonMixin.js:64–71 made it clear that this is what applies the blue color on hover and the styling around the link text. And as it is a button it wraps up the rectangular space.
Why this issue was not experienced before:
Previously, most usages of KRouterLink were inside Vuetify layouts such as VApp, VLayout, and VFlex. Vuetify adds its own global CSS rules to components. Essentially, vuetify's styles were overriding the button styles applied by KRouterLink. Inorder to valide my point over here , I did an experiment with KRouterLink:
- When the component is rendered in a normal blank page, the button-like styling is visible.
- When the same component is wrapped inside Vuetify, it appears as a normal link.
I may be missing something. If you also think this is an issue, I do have an approach in mind that i'd like to share.
There was a problem hiding this comment.
If you look at the KRouterLink documentation page, you can see that when it is rendered as a basic-link, it does not have this styling: https://design-system.learningequality.org/krouterlink something is going awry if this is the case.
| <script> | ||
| import MessageLayout from '../../components/MessageLayout'; | ||
| import StudioMessageLayout from '../../components/MessageLayout'; |
There was a problem hiding this comment.
blocking: Wrong import path - this still imports from MessageLayout (the old Vuetify component) instead of StudioMessageLayout.
Should be:
import StudioMessageLayout from '../../components/StudioMessageLayout';This defeats the purpose of this PR for AccountDeleted.vue - it's still using Vuetify.
There was a problem hiding this comment.
Sorry about that, my mistake
| @@ -0,0 +1,105 @@ | |||
| <template> | |||
There was a problem hiding this comment.
blocking: The acceptance criteria in issue #5631 state:
If there is no unit test suite, a new one is created. Do not use obsolete
@vue/test-utilsapproach. Instead, use@testing-library/vue.
This new component has no unit tests. Consider adding tests for:
- Rendering with required
headerprop - Rendering with optional
textprop - Default slot content
- Named
backslot override
There was a problem hiding this comment.
Thanks for the input, I will work on this 👍
2 participants
Summary
The following PR helps the initiative of removing Vuetify from Studio. This focuses on the accounts page, where previously,
MessageLayouta vuetify built component was used. I have designed another component,StudioMessageLayout, which is built usingStudioPageand custom styling. The PR has not explicitly removedMessageLayoutor affected its present behavior. Instead ,its usage has been removed and replaced in the following files:accounts/pages/accountDeleted/AccountDeleted.vueaccounts/pages/activateAccount/AccountCreated.vueaccounts/pages/activateAccount/AccountNotActivated.vueaccounts/pages/activateAccount/ActivationExpired.vueaccounts/pages/activateAccount/ActivationLinkReSent.vueaccounts/pages/activateAccount/ActivationSent.vueaccounts/pages/resetPassword/PasswordInstructionsSent.vueaccounts/pages/resetPassword/ResetLinkExpired.vueaccounts/pages/resetPassword/ResetPasswordSuccess.vuescreenshots
Since adding all the images in the PR would make it lengthy, I have uploaded all of them with labels in the following google-drive link: Studio issue#5631
References
Closes #5631
Reviewer guidance
The changes are reflected in the following files. After going to the login page, adding the following URL will take to the page:
AccountDeleted.vue= /account-deletedAccountCreated.vue= /account-createdAccountNotActivated.vue= /account-not-activeActivationExpired.vue= /activation-expiredActivationLinkReSent.vue= /activation-resentActivationSent.vue= /activation-sentPasswordInstructionsSent.vue= /password-reset-sentResetLinkExpired.vue= /reset-expiredResetPasswordSuccess.vue= /password-reset-success…