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

Replace TabsWithOverflow with KListWithOverflow #12035

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

AllanOXDi
Copy link
Member

Summary

References

#12011

Reviewer guidance

Do the changes look good to you?

Testing checklist

  • 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
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • 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

@github-actions github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend labels Mar 29, 2024
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Looks like the version of KDS on develop doesn't yet have the KListWithOverflow in it.

@MisRob -- is there any reason we're not using KDS >= v4.0.0 on develop? If so then this is as good a place as any to make that update eh?

@MisRob
Copy link
Member

MisRob commented Apr 2, 2024

is there any reason we're not using KDS >= v4.0.0 on develop? If so then this is as good a place as any to make that update eh?

@nucleogenesis KDS v4 is the release containing rebranding and it will be introduced in this PR #11911, at first to the release branch (and from there merged to the develop I assume)

KListWithOverflow is also available in KDS v3.1.1 so it can be installed into branches that have no branding changes yet. So I think you could just install 3.1.1 in this PR.

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

The code switch looks super close but there is one thing that is left behind with the component swap.

The KTabsList is used in the old TabsWithOverflow and that is where we get our tab & click handling business from.

I think that might fix the styling issues as well, but if not, I hope the fix there is straightforward.

@@ -39,49 +39,42 @@
:layout8="{ span: 6 }"
:layout12="{ span: 10 }"
>
<TabsWithOverflow
<KListWithOverflow
Copy link
Member

Choose a reason for hiding this comment

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

When I tested it I noticed we're missing the styles and click-ability of the section buttons. I believe this is because the TabsWithOverflow component wrapped everything within a KTabsList.

I think that you'll need to still migrate some of that logic to this component - such as the "activeTabId" and "@click" handler and possibly the "tabsWrapper" ref.

I think that it may work by wrapping the whole <template #item="{ item }"> block with the KTabsList component without worrying about the #tab slot there -- but I'm not super sure on this.

tabindex="-1"
class="overflow-tabs"
icon="optionsHorizontal"
:style="overflowButtonStyles(overflowTabs)"
:style="overflowButtonStyles(overflowItems)"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the overflowItems given by the new KListWithOverflow are the same shape as the data we were getting before because of the missing styles -- however, this may only be due to the missing KTabsList component issue mentioned above.

@MisRob
Copy link
Member

MisRob commented Apr 17, 2024

@AllanOXDi @nucleogenesis I'm not familiar with the details of work, just noticed that currently it seems we're moving away from KTabsList to KListWithOverflow + plus some custom logic. Just an alert that KListWithOverflow can't replace KTabsList by no means since KTabsList contains tons of logic related to a11y specific to tabbed interfaces whereas KListWithOverflow is much more general. My recommendation would be to return it and also invite @radinamatic as soon as work here is done to give final confirm.

Relatedly, KTabsList will be updated to contain KListWithOverflow within itself at some point because that will resolve lots of issues across the whole Kolibri where we see tabs taking two or more rows. If it would help with this task, we could prioritize it and rather focus on updating KTabsList to support this behavior by default. I don't think it'd be complicated work, it's something we need to do anyways.

@nucleogenesis
Copy link
Member

This issue is blocked by this KDS issue - some of my feedback above re: tab styles/functionality will be added directly into KDS, so once that issue is closed, we can update this PR accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quiz Creation: Replace TabsWithOverflow with KListWithOverflow
3 participants