ic-ui-kit icon indicating copy to clipboard operation
ic-ui-kit copied to clipboard

Bugfixes for dynamic tab panels - feedback requested

Open GCHQ-Developer-299 opened this issue 1 year ago • 2 comments

Summary of the changes

Significant refactor of tab components - internal props removed from IcTabPanel, because they were managed by IcTabContext already.

The ticket identifies a number of issues:

Tab panel appears above the tabs

Fixed - but with the addition of an empty <span> inside the 'Dynamic Rendering' story's IcTabContext. I think a proper fix to this might require enforcing slots in IcTabContext, so it can enforce the order that TabGroup and TabPanels render in?

The first tab is not selected

Fixed!

If the second tab is selected and another tab is dynamically removed, the second tab loses the selected state and the first tab is selected

Unable to replicate!

All tabPanels are displayed when tabs are dynamically loaded and a tab is selected

Unable to resolve - I spent a long time looking into this. The issue seems to be that newly created tabs aren't given the hidden='true' attribute and that's why they all appear when a new tab is selected.

I think it's down to the IcTabContext not setting them up correctly - but i spent a significant amount of time trying to adjust configureTabs() and linkTabs() - and even hard-setting hidden to true on new tab panels didn't stick, they would still render when a new tab was selected

Related issue

#1799

Checklist

Testing

  • [x] Relevant unit tests and visual regression tests added.
  • [x] Playground stories in React Storybook up to date, with any prop changes and additions addressed.
  • [x] Compare performance of modified components against develop using Performance addon in React Storybook.

Accessibility

  • [x] Accessibility Insights FastPass performed.
  • [x] A11y plug-in on Storybook yields no issues.
  • [x] Manual screen reader testing performed using NVDA and VoiceOver.
  • [x] Manual keyboard testing for keyboard controls and logical focus order.
  • [x] Correct roles used and ARIA attributes used correctly where required.
  • [x] Logical heading structure is maintained, and the HTML elements used for headings can be changed to fit within the wider page structure.

Testing content extremes

  • [x] Min/max content examples tested with no loss of content or overflow.
  • [x] All prop combinations work without issue.
  • [x] Tested for FOUC (Flash of Unstyled Content) in both SSR (Server-Side Rendering) and SSG (Static Site Generation) settings.
  • [x] Controlled and uncontrolled input components tested.
  • [x] Props/slots can be updated after initial render.

GCHQ-Developer-299 avatar Oct 01 '24 10:10 GCHQ-Developer-299

View your branch deployment here: https://mi6.github.io/ic-ui-kit/branches/1799-dynamic-tab-panels/web-components View your React branch deployment here: https://mi6.github.io/ic-ui-kit/branches/1799-dynamic-tab-panels/react

github-actions[bot] avatar Oct 01 '24 10:10 github-actions[bot]

Tab panel appears above the tabs yep, appears fixed. seems reasonable as otherwise we need a breaking change i guess to resolve as you suggest

The first tab is not selected Fixed

If the second tab is selected and another tab is dynamically removed, the second tab loses the selected state and the first tab is selected Also unable to replicate!

All tabPanels are displayed when tabs are dynamically loaded and a tab is selected Having looked as this too (a while ago). It seems to be a timing issue with iterating over the same array of elements to render tab and the tap-panel. I'm not really sure what can be done to resolve this

ad9242 avatar Oct 10 '24 12:10 ad9242

A few comments - generally agree with @ad9242:

Tab panel appears above the tabs
 I agree that adding slots for the TabGroup and TabPanels would be a way to fix this properly in future, and that the current fix is reasonable.

The first tab is not selected
 Yep, agree that this is fixed.


If the second tab is selected and another tab is dynamically removed, the second tab loses the selected state and the first tab is selected
 I am able to replicate this in the StackBlitz provided in the ticket (although only after refreshing the whole page, and only sometimes!). But I think the changes in this PR have fixed it - I am not able to replicate it in the story that has been added.


All tabPanels are displayed when tabs are dynamically loaded and a tab is selected
 Not sure what can be done about this either - I would need to spend a lot more time looking into it in detail before I can suggest anything really

GCHQ-Developer-847 avatar Oct 11 '24 17:10 GCHQ-Developer-847