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

[Accessibility-3100036]: Close button is nested under 'Home' tab control under 'Data Explorer' pane. #1818

Merged
merged 7 commits into from
Jun 5, 2024

Conversation

satya07sb
Copy link
Collaborator

@satya07sb satya07sb commented Apr 24, 2024

Preview this branch

In the previous implementation within the 'Tabs' section, we provided full clickable access, allowing users to click on tabs containing a close button.

In this update, we've enhanced the user experience by differentiating the 'Home' section and the close button section. We achieved this by enclosing the 'Home' section within its own wrap, while the close button remains separate. As a result, all Home tab controls now exclusively belong to the 'Home' section, providing a more streamlined navigation experience. Additionally, the close button maintains its distinct position, ensuring clarity and intuitive interaction for users.

…lose button is nested under 'Home' tab control under 'Data Explorer' pane.
@satya07sb satya07sb marked this pull request as ready for review April 24, 2024 10:40
@satya07sb satya07sb requested a review from a team as a code owner April 24, 2024 10:40
asierisayas
asierisayas previously approved these changes Apr 24, 2024
Copy link
Contributor

@sevoku sevoku left a comment

Choose a reason for hiding this comment

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

Unfortunately this breaks the design in Fabric:
image

Fabric specific style is here:

.nav-tabs>li>.tabNavContentContainer>.tab_Content:hover {
border-bottom: 2px solid #e0e0e0;
}
.nav-tabs>li.active>.tabNavContentContainer>.tab_Content,
.nav-tabs>li.active>.tabNavContentContainer>.tab_Content:hover {
border-bottom: 2px solid @FabricAccentMedium;
}
.nav-tabs>li.active>.tabNavContentContainer>.tab_Content>.tabNavText {
border-bottom: 0px none transparent;
}
.tabNavContentContainer {
padding: @SmallSpace 0px @SmallSpace 0px;
&:hover {
background-color: transparent;
border-color: transparent;
}
.tab_Content {
border-right: 0px none transparent;
margin: 0px @SmallSpace 0px @SmallSpace;
width: calc(@TabsWidth - (@SmallSpace * 2));
padding-bottom: @SmallSpace;
.statusIconContainer {
margin-left: 0px;
}
.tabIconSection {
.cancelButton {
padding: 0px 0px 0px @SmallSpace;
&:hover {
background-color: transparent;
}
&:focus {
background-color: transparent;
}
&:active {
background-color: transparent;
}
}
}
}
}

sevoku
sevoku previously approved these changes Apr 26, 2024
Copy link
Contributor

@sevoku sevoku left a comment

Choose a reason for hiding this comment

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

This works, thanks!

Copy link
Contributor

@vchske vchske left a comment

Choose a reason for hiding this comment

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

This breaks being able to close at least one tab (Scale and Settings). Notice how far out the close button is:
image
Although you can't see the pointer I am hovering over where the close button should be here:
image

But because the close button was outside the tab, the tab to the right (Items) now covers the close button.

@satya07sb satya07sb dismissed stale reviews from sunghyunkang1111 and sevoku via 2622827 April 26, 2024 20:47
@satya07sb satya07sb requested a review from vchske April 26, 2024 21:00
vchske
vchske previously approved these changes Apr 26, 2024
Copy link
Contributor

@vchske vchske left a comment

Choose a reason for hiding this comment

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

Change fixed the close button. Thanks!

One more thing, when using the tab key to navigate the tabs, when I tab to, say "....Items", I can hit the spacebar and the Items tab is now focused. But when I tab back to the "Home" tab and I hit the spacebar, the Home tab is not brought to focus, even though the text of tab, "Home" is underlined:
image
I'm not sure if this is problematic enough to hold this checkin but wanted to point it out.

@satya07sb
Copy link
Collaborator Author

Change fixed the close button. Thanks!

One more thing, when using the tab key to navigate the tabs, when I tab to, say "....Items", I can hit the spacebar and the Items tab is now focused. But when I tab back to the "Home" tab and I hit the spacebar, the Home tab is not brought to focus, even though the text of tab, "Home" is underlined: image I'm not sure if this is problematic enough to hold this checkin but wanted to point it out.

You are correct, but this is an existing issue. The code change is not affecting this problem, and it is out of scope for this ticket. However, it will not take much effort to fix it, so let me address it as well.

@satya07sb satya07sb requested a review from languy May 17, 2024 07:11
Copy link
Contributor

@sevoku sevoku left a comment

Choose a reason for hiding this comment

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

Retested with Fabric and everything looks fine.

@satya07sb satya07sb merged commit 52c2cfe into master Jun 5, 2024
20 checks passed
@satya07sb satya07sb deleted the accessibility-3100036 branch June 5, 2024 15:12
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.

None yet

5 participants