Skip to content

refactor(NcAppSidebarTabs): use custom components for tab buttons - #7516

Merged
susnux merged 3 commits into
mainfrom
refactor/nc-app-sidebar-tabs
Oct 7, 2025
Merged

refactor(NcAppSidebarTabs): use custom components for tab buttons#7516
susnux merged 3 commits into
mainfrom
refactor/nc-app-sidebar-tabs

Conversation

@susnux

@susnux susnux commented Sep 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

☑️ Resolves

NcCheckboxRadioSwitch with button styling is deprecated. The tabs also had overridden styles so instead of reusing the radio component and hacking styles we create a small custom component for this use case.
Also the wcag examples use buttons and not radio.

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 2️⃣ Backport to stable8 for maintained Vue 2 version or not applicable

@susnux susnux added this to the 9.0.0-rc.10 milestone Sep 15, 2025
@susnux susnux added 3. to review Waiting for reviews feature: app-sidebar Related to the app-sidebar component refactor ♻️ Pull request that is neither a fix nor a feature labels Sep 15, 2025
@susnux
susnux force-pushed the refactor/nc-app-sidebar-tabs branch from 6865af7 to d900977 Compare September 15, 2025 13:49
@szaimen
szaimen requested review from a team and removed request for szaimen September 15, 2025 13:51
@susnux
susnux force-pushed the refactor/nc-app-sidebar-tabs branch from b7f788b to 585bb8f Compare September 15, 2025 16:13
ShGKme
ShGKme reviewed Sep 15, 2025
View reviewed changes

@ShGKme ShGKme left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cool, always wanted to remove NcChekboxRadioSwitch here in flavour of buttons.

Some nitpiks.

Comment thread src/components/NcAppSidebar/NcAppSidebarTabsButton.vue Outdated
Comment thread tests/unit/components/NcAppSidebar/NcAppSidebarTabs.spec.js

@ShGKme ShGKme Sep 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why?

@susnux susnux Sep 23, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

moved to component tests

Comment thread tests/component/components/NcAppSidebar/AppSidebarTabs.story.vue
Comment thread src/components/NcAppSidebar/NcAppSidebarTabs.vue Outdated
Comment thread src/components/NcAppSidebar/NcAppSidebarTabsButton.vue Outdated
ShGKme
ShGKme reviewed Sep 15, 2025
View reviewed changes
Comment thread src/components/NcAppSidebar/NcAppSidebarTabsButton.vue Outdated
ShGKme
ShGKme reviewed Sep 15, 2025
View reviewed changes
Comment thread src/components/NcAppSidebar/NcAppSidebarTabsButton.vue Outdated
ShGKme

This comment was marked as resolved.

Sign in to view

kra-mo

This comment was marked as off-topic.

Sign in to view

@kra-mo

This comment was marked as off-topic.

Sign in to view
jancborchardt

This comment was marked as resolved.

Sign in to view

@ShGKme

This comment was marked as resolved.

Sign in to view
@jancborchardt jancborchardt moved this to 🏗️ At engineering in 🖍 Design team Sep 16, 2025
@ShGKme

This comment was marked as resolved.

Sign in to view
@kra-mo

This comment was marked as resolved.

Sign in to view
@Antreesy

This comment was marked as resolved.

Sign in to view
@kra-mo kra-mo mentioned this pull request Sep 16, 2025
Closed
@kra-mo

This comment was marked as resolved.

Sign in to view
@susnux

susnux commented Sep 20, 2025

Copy link
Copy Markdown
Contributor Author

The design has changed

Yes wrong order during rebases, this PR is not supposed to change any design.
It should only refactor internal component usage.

@susnux
susnux dismissed kra-mo’s stale review September 20, 2025 09:49

no design changes - will do in a follow up

@susnux
susnux force-pushed the refactor/nc-app-sidebar-tabs branch from 0e32aab to d65a5f2 Compare September 20, 2025 10:00
@susnux
susnux requested a review from ShGKme September 20, 2025 10:13
@susnux

susnux commented Sep 20, 2025

Copy link
Copy Markdown
Contributor Author

Should now be visually the same as before.

@ShGKme

This comment was marked as resolved.

Sign in to view
susnux
susnux commented Sep 20, 2025
View reviewed changes
Comment thread src/components/NcAppSidebar/NcAppSidebarTabsButton.vue
ShGKme
ShGKme requested changes Sep 24, 2025
View reviewed changes
Comment thread src/components/NcAppSidebar/NcAppSidebarTabsButton.vue
@susnux
susnux force-pushed the refactor/nc-app-sidebar-tabs branch from 5bfb8fd to cbfa155 Compare September 24, 2025 15:55
@susnux

susnux commented Sep 24, 2025

Copy link
Copy Markdown
Contributor Author

rebase with autosquash.

@Antreesy Antreesy modified the milestones: 9.0.0-rc.10, 9.0.1 Oct 1, 2025
jancborchardt
jancborchardt reviewed Oct 6, 2025
View reviewed changes

@jancborchardt jancborchardt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there an updated screenshot for a quick review? But if @kra-mo ’s feedback was addressed, I’d say it’s good to go. :)

@ShGKme

ShGKme commented Oct 6, 2025

Copy link
Copy Markdown
Contributor

@jancborchardt This is an internal implementation refactoring. Not visual component update. There supposed to be no visual changes.

Antreesy
Antreesy reviewed Oct 6, 2025
View reviewed changes
Comment thread src/components/NcAppSidebar/NcAppSidebarTabsButton.vue
Antreesy
Antreesy approved these changes Oct 6, 2025
View reviewed changes

@Antreesy Antreesy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Otherwise good, tested against Talk app

@susnux susnux modified the milestones: 9.0.1, 9.0.2 Oct 6, 2025
susnux added 3 commits October 7, 2025 15:35
@susnux
refactor(NcAppSidebarTabs): use custom components for tab buttons
755576e 
NcCheckboxRadioSwitch with button styling is deprecated.
The tabs also had overridden styles so instead of reusing the radio
component and hacking styles we create a small custom component for this
use case.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
test(NcSidebarTabs): migrate to playwright tests for proper user tests
123c29c 
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
refactor(NcAppSidebarTabs): migrate to Intl
4b4b494 
This will make playwright tests work without mocking OC.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
susnux force-pushed the refactor/nc-app-sidebar-tabs branch from cbfa155 to 4b4b494 Compare October 7, 2025 13:35
@susnux

susnux commented Oct 7, 2025

Copy link
Copy Markdown
Contributor Author

rebased

@susnux
susnux merged commit b07c4ef into main Oct 7, 2025
25 checks passed
@susnux
susnux deleted the refactor/nc-app-sidebar-tabs branch October 7, 2025 13:42
@susnux susnux modified the milestones: 9.0.2, 9.1.0 Oct 14, 2025
@ShGKme ShGKme mentioned this pull request Oct 22, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jancborchardt jancborchardt jancborchardt left review comments
@ShGKme ShGKme ShGKme approved these changes
@Antreesy Antreesy Antreesy approved these changes
@kra-mo kra-mo Awaiting requested review from kra-mo

Assignees

No one assigned

Labels

3. to review Waiting for reviews feature: app-sidebar Related to the app-sidebar component refactor ♻️ Pull request that is neither a fix nor a feature

Projects

None yet

Milestone

9.1.0

Development

Successfully merging this pull request may close these issues.

5 participants

@susnux @kra-mo @ShGKme @Antreesy @jancborchardt