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.

Sign up for GitHub

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

Jump to bottom

Fix tabs appearance #4815

Merged
merged 1 commit into from
Nov 14, 2023
Merged

Fix tabs appearance #4815

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions src/components/NcAppSidebar/NcAppSidebarTabs.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,8 +269,8 @@ export default {
&__nav {
display: flex;
justify-content: stretch;
margin-top: 10px;
padding: 0 4px;
margin: 10px 8px 0 8px;
border-bottom: 1px solid var(--color-border);
}

&__tab {
Expand Down Expand Up @@ -314,7 +314,23 @@ export default {
}
}

:deep(.checkbox-radio-switch--button-variant.checkbox-radio-switch) {
border: unset;
// Override checkbox-radio-switch styles so that it looks like tabs
:deep(.checkbox-radio-switch--button-variant) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This completely changes the UI of the component, should we instead change that component?
Because this is now very likely to break on slight changes of the internals of NcCheckboxRadioSwitch.
For the same reason I feel quite uncomfortable with that the many !important statements.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we first learn what the intended goal is here? Do we intend to go back with the design completely? Is it just about small fixes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There has been a last minute request to revert this, sorry. It's a last minute fix that concerns only the sidebar tabs, hence the deep and important. It's temporary, not permanent. We can discuss what to do with checkboxradioswitch later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said, I think we're trying to do way too many things with NcCheckboxRadioSwitch

Copy link
Contributor

Choose a reason for hiding this comment

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

@marcoambrosini can you make sure it doesn’t break in these 2 places:

  • Sharing flow where we use the component vertically (right?)
  • Forms on top right, where we use it for View/Edit/Results

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we first learn what the intended goal is here? Do we intend to go back with the design completely? Is it just about small fixes?

@raimund-schluessler Yes to both: It is about small fixes, and the active tab being way too present has been identified as a visual issue. Going to a middle ground between the old and new one seems a good way to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure this won't cause a11y issues? Cc @JuliaKirschenheuter

Copy link
Contributor

@ShGKme ShGKme Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Are we sure this won't cause a11y issues? Cc @JuliaKirschenheuter

It only changes styles, so the only possible a11y issues could be about state styles and contrast. With the contrast everything is fine.

But with states it is not.

The current tab should have hover and active state styles.

Before After
with-state no-state

border: unset !important;
border-radius: 0 !important;
.checkbox-content {
padding: var(--default-grid-baseline);
border-radius: var(--default-grid-baseline) var(--default-grid-baseline) 0 0 !important;
margin: 0 !important;
border-bottom: var(--default-grid-baseline) solid transparent !important;
.checkbox-content__icon--checked > * {
color: var(--color-main-text) !important;
}
}
&.checkbox-radio-switch--checked .checkbox-radio-switch__content{
background: transparent !important;
color: var(--color-main-text) !important;
border-bottom: var(--default-grid-baseline) solid var(--color-primary-element) !important;
}
}
</style>
Loading