[Remove Vuetify from Studio] Saved searches dialogs #5604
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! 😊 |
|
Season’s greetings! 👋 We’d like to thank everyone for another year of fruitful collaborations, engaging discussions, and for the continued support of our work. Learning Equality will be on holidays from December 22 to January 5. We look forward to much more in the new year and wish you a very happy holiday season! Are you preparing for Google Summer of Code? See our GSoC guidelines. |
MisRob
changed the title
There was a problem hiding this comment.
Hi @JanaKocakova, this is very nice work, thanks a lot!
I confirm all works as expected. Code changes are meaningful in all areas - semantics, RTL, a11y. I see that you really paid attention to guidance and spend enough time getting to know the codebase - much appreciated. Excellent first contribution.
I really don't have much to add except the area you are asking about - colors. So, the way it works with KDS is described in detail here https://design-system.learningequality.org/colors. You'd want to remove colors from CSS section completely and instead use dynamic :style as described in this section https://design-system.learningequality.org/colors#usage. For each color, I will leave an additional comment about which token we'd want to use. Hopefully the documentation is clear, but you could also find many examples in the codebase if that'd help. Let me know if you had any questions.
| </div> | ||
|
|
||
| <div class="search-actions"> | ||
| <IconButton |
There was a problem hiding this comment.
Would you please replace by KIconButton? I think using the proxy component in this case doesn't have any additional impact (and later as part of migrating to KDS fully, we will remove IconButton anyway)
| align-items: center; | ||
| justify-content: space-between; | ||
| padding: 16px 0; | ||
| border-bottom: 1px solid rgba(0, 0, 0, 0.12); |
There was a problem hiding this comment.
| .empty-state { | ||
| padding: 8px; | ||
| color: var(--v-grey-base); |
There was a problem hiding this comment.
| <style scoped lang="scss"> | ||
| .empty-state { | ||
| padding: 8px; |
There was a problem hiding this comment.
Would you remove this padding? I realize that you used it to preserve the original experience as requested - thanks for paying attention to that :)
I just think that here it'd be more meaningful to rely on default KModal alignment (even though in some cases we make exceptions, generally one of the purposes of KDS components in consistency).
| .metadata { | ||
| font-size: 14px; | ||
| color: var(--v-grey-darken2); |
There was a problem hiding this comment.
https://design-system.learningequality.org/colors#tokens-annotation
Same token will replace var(--v-grey-base); currently used for the separator dot (defined below on line 221).
|
@JanaKocakova In addition to our team-wide holiday time off (see comment above), I wanted to mention that I will have an additional week off and will be back on January 12. So take your time and I'll be in touch again after I'm back. Šťastné a veselé 🎄 and happy new year 😉 |
|
Oh also @JanaKocakova, lint check is failing. Can you run |
Yes, that makes total sense. Thank you for mentioning that. |
2 participants
Fixes #5470
Summary
This PR migrates the two dialogs in SavedSearchesModal from Vuetify components to KDS:
MessageDialogwithKModalfor delete confirmation dialogActionLinkwithKRouterLinkfor saved search linksVList,VListTile*components with semantic HTML (<ul>,<li>,<div>) and custom SCSSVDividerwith CSSborder-bottomstylingnotranslateclass to user-generated search names to prevent Chrome auto-translationManual verification performed:
References
Parent issue - #5060)
Reviewer guidance