-
Notifications
You must be signed in to change notification settings - Fork 59
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
Add stepped class in wizard/tabs #1489
base: dev
Are you sure you want to change the base?
Conversation
8dfe5c4
to
86bf897
Compare
Accessibility Violations Found
|
Lighthouse scores (desktop)
|
Lighthouse scores (mobile)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
Accessibility Violations Found
|
5fb8969
to
3180c1d
Compare
Accessibility Violations Found
|
Lighthouse scores (desktop)
|
Lighthouse scores (mobile)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
Accessibility Violations Found
|
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1489 +/- ##
=========================================
Coverage 82.33% 82.33%
Complexity 963 963
=========================================
Files 107 107
Lines 2485 2485
Branches 339 339
=========================================
Hits 2046 2046
Misses 264 264
Partials 175 175 ☔ View full report in Codecov by Sentry. |
ui.frontend/src/view/FormTabs.js
Outdated
@@ -116,6 +116,11 @@ class FormTabs extends FormPanel { | |||
tabs[i].setAttribute(Constants.TABINDEX, "0"); | |||
tabs[i].setAttribute(Constants.ARIA_CURRENT, "true"); | |||
} else { | |||
// If the panel/tab was previously active, mark it as stepped | |||
if (tabpanels[i].classList.contains(this.#_selectors.active.tabpanel)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you validate the following use-cases,
- If tabs are dynamically added/removed (via repeatable settings), this indexing might break. Please add test cases for this use-case
for all layout types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since it is being directly added to HTML, and is not something which is repopulated, so this might work.
Whenever we introduce lazy loading, at that time this will break.
Nevertheless, we should have a repeatable test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check comment
42b0ba2
to
17ae34f
Compare
@@ -232,7 +269,7 @@ describe('visibility of navigation buttons', function () { | |||
// check when new instance of repeatable panel is added | |||
// and that new instance gets active & prev and next nav buttons are visible | |||
cy.get(`#${textInputId}`).find(driverTextInput).focus().clear().type('a').blur().then(() => { | |||
getWizardPanelAtIndex(2).should('have.class', wizardPanelActive); | |||
getWizardPanelAtIndex(11).should('have.class', wizardPanelActive); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When there is no change in collateral, why is this test case changed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was failing on my local, because the panel at index 11 was active, and it was checking at index 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test case was working earlier, ideally if an old test case is changed, it is most likely due to collateral change else this could be a regression. please investigate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, reverted to old scenario!
|
||
it("After clicking on every tab, if tab-1 is repeated, the repeated instance should not have stepped class", () => { | ||
getTabs().should('have.length', 4); | ||
getWizardPanels().should('have.length', 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see code to add/remove instances in between the tabs/wizard, which was asked as per last review comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added the instances in tabs/wizards, you can check it in the test case as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check comments
getWizardPanelAtIndex(1).should('have.class', 'cmp-adaptiveform-wizard__wizardpanel--stepped'); | ||
getTabAtIndex(0).should('have.class', 'cmp-adaptiveform-wizard__tab--stepped'); | ||
getWizardPanelAtIndex(0).should('have.class', 'cmp-adaptiveform-wizard__wizardpanel--stepped'); | ||
cy.get("button").contains("+R1").click().then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rismehta I have added the instance, you can check it here!
de9c776
to
c94969f
Compare
b9a03cc
to
628c889
Compare
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: