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

feat(xo-core): update TabItem and TabList components to v2 #8047

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

OlivierFL
Copy link
Collaborator

Description

  • Update component name and CSS to match new component guidelines
  • Update component usages in XO 6 and XO Lite

Checklist

  • Commit
    • Title follows commit conventions
    • Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
    • If bug fix, add Introduced by
  • Changelog
    • If visible by XOA users, add changelog entry
    • Update "Packages to release" in CHANGELOG.unreleased.md
  • PR
    • If UI changes, add screenshots
    • If not finished or not tested, open as Draft

Review process

This 2-passes review process aims to:

  • develop skills of junior reviewers
  • limit the workload for senior reviewers
  • limit the number of unnecessary changes by the author
  1. The author creates a PR.
  2. Review process:
    1. The author assigns the junior reviewer.
    2. The junior reviewer conducts their review:
      • Resolves their comments if they are addressed.
      • Adds comments if necessary or approves the PR.
    3. The junior reviewer assigns the senior reviewer.
    4. The senior reviewer conducts their review:
      • If there are no unresolved comments on the PR → merge.
      • Otherwise, we continue with 3.
  3. The author responds to comments and/or makes corrections, and we go back to 2.

Notes:

  1. The author can request a review at any time, even if the PR is still a Draft.
  2. In theory, there should not be more than one reviewer at a time.
  3. The author should not make any changes:
    • When a reviewer is assigned.
    • Between the junior and senior reviews.

@OlivierFL OlivierFL self-assigned this Oct 11, 2024
@OlivierFL OlivierFL force-pushed the xo6/update-tabs-components branch from e413add to b30d16d Compare October 11, 2024 12:07
@OlivierFL OlivierFL requested a review from MathieuRA October 11, 2024 12:09
selected?: boolean
tag?: string
}>(),
{ tag: 'span', disabled: undefined }
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of assigning undefined as the default value?
Is it to prevent vue from converting undefined to false?
If so, is there any point in having undefined instead of false here?
Or maybe this is something I missed in the guideline?
In that case, why selected is not handled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is to prevent undefined to false conversion by Vue. In this case it is necessary because the disabled prop is used with useContext() on line 30. selected is not used in the context, so there is no need to do the same.
More information here

Comment on lines 16 to 20
defineProps<{
disabled?: boolean
selected?: boolean
tag?: string
}>(),
Copy link
Member

Choose a reason for hiding this comment

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

In the DS, i can see a counter prop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it is for ease of use in Figma, in the Vue component, we have a slot, so we can pass any component if we need to.

@@ -1,6 +1,6 @@
<!-- v1.0 -->
<!-- v2 -->
Copy link
Member

Choose a reason for hiding this comment

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

Based on the Tabs bar changelog, the v2 implement the scroll bar option

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the Vue component, the scrollbar is automatically visible when there is not enough space. I'm not sure if we should have a prop to always enable it.

@@ -22,7 +22,7 @@ export function useTabList<TName extends string>(names: TName[], initialTab?: Ma
event.preventDefault()
activate(name)
},
active: isActive(name),
selected: isActive(name),
Copy link
Member

Choose a reason for hiding this comment

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

selected: isSelected(name) maybe? In order to sync name. (or isCurrent() if the proposition is accepted)

@MathieuRA MathieuRA requested a review from ByScripts October 11, 2024 15:27
@OlivierFL OlivierFL force-pushed the xo6/update-tabs-components branch from b30d16d to 1693636 Compare December 4, 2024 09:48
@OlivierFL
Copy link
Collaborator Author

OlivierFL commented Dec 4, 2024

@MathieuRA @ByScripts
I’ve moved the components to the /ui directory. However, I have some questions regarding the UiTabList name.
There is a component in the DS called Tabs bar, and I'm wondering if I should rename the Vue component? If we follow our guidelines, it should be named as UiTabsBar, but for most of the components that handle a list of items, we named them as *List, so I don't know.

Also, I didn’t bump the UiTab version (to v3) because there is an issue with the :focus-visible style, as shown in the screenshot below:
image

I tried to fix this by changing the overflow value in UiTabList, but this causes an issue with the horizontal scrollbar we should have when the tab list is larger than its parent.
So for now, the :focus-visible style is the same as the :hover one. Maybe we should discuss this with @clemencebx

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.

2 participants