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 tabs components #420

Merged
merged 7 commits into from
Feb 28, 2023
Merged

Add tabs components #420

merged 7 commits into from
Feb 28, 2023

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Feb 8, 2023

Description

Adds new tabs components - KTabs, KTabsList, KTabsPanel. Please preview new documentation pages to understand intended usage:

  • Tabs page has general information and recommendations for all new tab components
  • KTabs, KTabsList, and KTabsPanel pages contain more specific information that wasn't mentioned on the general tabs page

Motivated by learningequality/kolibri#9849

Issue addressed

Addresses #385

Steps to test

  • Preview examples on documentation pages referenced above
  • Run this Kolibri PR and test these Coach Reports tabs that were refactored to use new KDS tab components (you don't need to review the code of the Kolibri PR as it's not intended for merge - it will be used as a basis of another PR and needs to be adjusted):

Screenshot from 2023-02-13 18-03-49

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)?

Comments

There's still one a11y problem in Kolibri that is resolved only partially (see learningequality/kolibri#10113 and its "Comments" section). @radinamatic I was able to deal with it to large extent in the Kolibri test PR, however, one disadvantage is that the focus now automatically goes to the active tab even when you arrive on the page for the first time, which I assume is not expected. It seems that overall it is still better than the current version, however, I'd appreciate if you could please test it out and let me know how big a problem that is. If it's important, I believe we'd need to prioritize learningequality/kolibri#10113.

by increasing the timeout value.

The problem was that the keyboard
modality got reset to `false` before
an element could receive focus
due to very short timeout value.
@MisRob
Copy link
Member Author

MisRob commented Feb 13, 2023

@jtamiace I'd appreciate your feedback on four new KDS pages referenced in the PR summary

@marcellamaki
Copy link
Member

@MisRob I have started looking this over and reviewing the documentation pieces. I have a few other things to work on in the time remaining today, but plan to complete my review on Monday and will write up my comments then.

@jtamiace
Copy link
Contributor

Thanks @MisRob, I think it's great to have the visual example of the tabs component in the Tabs page. Would the sections with code in the Tabs component page be able to fit under any of the Code library component pages KTabs, KTabsList, and KTabsPanel? The pages under the Components section are more geared towards UX guidelines and how to use a component in a design. Instead, you could use cross-links from the Tabs page, similar to how it's done in the Buttons and Modals pages.

I'd like to do more thinking on what can be included in the Components > Tabs page, but off the top of my head, the types of information that would be appropriate for this page are guidelines around when to use and not use tabs, how much text goes in a tab, how the component should be used in relation to other elements, overflow, visual specs for different tab states, and any other "do's and don'ts" kind of advice

@MisRob
Copy link
Member Author

MisRob commented Feb 22, 2023

Thank you @jtamiace

Would the sections with code in the Tabs component page be able to fit under any of the Code library component pages KTabs, KTabsList, and KTabsPanel? The pages under the Components section are more geared towards UX guidelines and how to use a component in a design. Instead, you could use cross-links from the Tabs page, similar to how it's done in the Buttons and Modals pages.

That makes sense. I will try to re-organize existing documentation to library pages and share the updated documentation with you again.

I'd like to do more thinking on what can be included in the Components > Tabs page, but off the top of my head, the types of information that would be appropriate for this page are guidelines around when to use and not use tabs, how much text goes in a tab, how the component should be used in relation to other elements, overflow, visual specs for different tab states, and any other "do's and don'ts" kind of advice

What would be the next steps regarding the preparation of these general guidelines?

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.

Hi @MisRob - thank you very much for your work on this.

Overall, I think the code is great and well commented - I especially appreciate your inclusion of some references. I do find the KTabsList component a bit dense, but having read it through a few times, it does get easier to understand with time, and I think the use case is just complex... I don't think it is a reflection of the code. Perhaps in a future KDS tactical or dev meeting, you could do a small code walk through :)

I don't have any code concerns if manual QA passes

lib/tabs/KTabs.vue Show resolved Hide resolved
lib/tabs/KTabsList.vue Show resolved Hide resolved
lib/tabs/utils.js Outdated Show resolved Hide resolved
@MisRob
Copy link
Member Author

MisRob commented Feb 23, 2023

Thank you @marcellamaki.

Regarding

I do find the KTabsList component a bit dense, but having read it through a few times, it does get easier to understand with time, and I think the use case is just complex... I don't think it is a reflection of the code. Perhaps in a future KDS tactical or dev meeting, you could do a small code walk through :)

That's a good note. I'd be happy to do the walk-through, and perhaps we can also see if we can get some ideas to simplify its structure on that opportunity.

@MisRob
Copy link
Member Author

MisRob commented Feb 24, 2023

All feedback is resolved and manual testing is completed as well learningequality/kolibri#10109

@jtamiace
Copy link
Contributor

For the general design guideline pages, the process we've used before is for a designer to draft and review text in gdocs. Started one here. I don't want to block you from merging this work in case the components are needed for 0.16 or other projects. I can prioritize the guideline page if there's a date you were looking to get this in by, or can work on this page as a separate PR if that makes more sense.

@MisRob
Copy link
Member Author

MisRob commented Feb 28, 2023

@jtamiace Thank you. I think it'd be better to add more information to the guidelines page in a separate PR, as we need these components in Kolibri soon. I'm pretty confident that new pieces of documentation won't affect their implementation and technical documentation and will be rather an enrichment to what we have already. You don't need to hurry, let me know any time, and I will then update the page.

@MisRob
Copy link
Member Author

MisRob commented Feb 28, 2023

Everything that could be done/tested/discussed at this point is complete. If there are no new notes, it'd be good to get this in so that I can update Kolibri with the correct KDS reference.

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.

nice work @MisRob!

@marcellamaki marcellamaki merged commit 893486a into learningequality:develop Feb 28, 2023
@MisRob MisRob deleted the tabs branch October 14, 2023 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants