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

fix(web-components): extra check to default & remove dupe conditional #2182

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

evenstensberg
Copy link
Contributor

@evenstensberg evenstensberg commented Jul 18, 2024

Summary of the changes

Wasn't able to reproduce the bug, but I believe an extra check in the render should do the trick. If you can show me code to make the reproduction viable, I can adjust the PR to that (https://github.com/evenstensberg/banner-repro-ic/blob/master/src/HelloWorldComponent.js)

Related issue

#2177

Checklist

General

  • Changes to docs package checked and committed.
  • All acceptance criteria reviewed and met.

Testing

  • Relevant unit tests and visual regression tests added.
  • Visual testing against Figma component specification completed.
  • Playground stories in React Storybook up to date, with any prop changes and additions addressed.

Accessibility

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

Resize/zoom behaviour

  • Page can be zoomed to 400% with no loss of content.
  • Screen magnifier used with no issues.
  • Text resized to 200% with no loss of content.
  • Text spacing increased as per the WCAG 1.4.12 success criterion with no loss of content.

System modes

  • Browser setting 'prefers reduced motion' tested. No animations or motion visible whilst this setting is on.
  • Windows High Contrast mode tested with no loss of content.
  • System light and dark mode tested with no loss of content.
  • Browser support tested (Chrome, Safari, Firefox and Edge).

Testing content extremes

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

@GCHQ-Developer-530
Copy link
Contributor

Hi, this PR has been inactive for 2 weeks now. Please leave a comment if you're still working on it. Continued inactivity will result in us closing this PR

@evenstensberg
Copy link
Contributor Author

@GCHQ-Developer-530 I can fix it, but I can't reproduce the error. Once a valid reproduction instruction/repo is made, the change would be trivial.

@GCHQ-Developer-530
Copy link
Contributor

To recreate:

  1. Update a storybook story to be
<IcClassificationBanner
      classification="official"
      inline="true"
      country={undefined}
    />
  1. Run storybook
  2. See that the classification banner now says "UNDEFINED OFFICIAL"

@GCHQ-Developer-530
Copy link
Contributor

Only a couple more things to sort out and this PR should be good. Please can you squash your commits into one. Also the PR is currently failing due to some lint errors, on line 45 and 46 it's giving this error - "Irregular whitespace not allowed no-irregular-whitespace", please could you update the PR to fix that? Thank you

@evenstensberg evenstensberg marked this pull request as ready for review July 30, 2024 15:19
@evenstensberg evenstensberg force-pushed the fix/2177 branch 2 times, most recently from 305ba4a to 07bf06e Compare July 30, 2024 17:01
@evenstensberg evenstensberg changed the title fix(web-components): extra check to default fix(web-components): extra check to default & remove dupe conditional Jul 30, 2024
@GCHQ-Developer-530
Copy link
Contributor

This is looking good code wise now! Would it be possible for you to add a spec test or a Cypress test where you pass through undefined and expect it to resolve to ""? Just so we'd know if we broke the behaviour in the future, and to keep the test coverage up. Thank you

@evenstensberg
Copy link
Contributor Author

PTAL

@evenstensberg evenstensberg force-pushed the fix/2177 branch 5 times, most recently from 977b2b8 to 60a9740 Compare August 2, 2024 19:03
@GCHQ-Developer-530
Copy link
Contributor

Please can you add another test where the props are set to undefined as well? Also please squash your commits into one. Thank you

@evenstensberg evenstensberg force-pushed the fix/2177 branch 2 times, most recently from 82f01fd to 3a2e1c6 Compare August 5, 2024 09:40
@evenstensberg
Copy link
Contributor Author

@GCHQ-Developer-530 I’ve also added a cypress test to make coverage better.

Copy link
Contributor

@GCHQ-Developer-530 GCHQ-Developer-530 left a comment

Choose a reason for hiding this comment

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

Looks good to me 😊 thanks again for your contribution!

Copy link
Contributor

@GCHQ-Developer-112 GCHQ-Developer-112 left a comment

Choose a reason for hiding this comment

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

Hi @evenstensberg , please can you update your commit messages so they are more descriptive of the changes made

@evenstensberg evenstensberg force-pushed the fix/2177 branch 2 times, most recently from 05bf4c5 to 8cb54ec Compare August 5, 2024 13:40
@evenstensberg
Copy link
Contributor Author

Hi @evenstensberg , please can you update your commit messages so they are more descriptive of the changes made

Pushed new commit messages now. Let me know if they are too vague @GCHQ-Developer-112 .

@GCHQ-Developer-112
Copy link
Contributor

Hi @evenstensberg , please can you update your commit messages so they are more descriptive of the changes made

Pushed new commit messages now. Let me know if they are too vague @GCHQ-Developer-112 .

The react commit looks good!

Please add "in classification banner" to the end of the current commit message for web components. If this is too many characters, "classification" can be shortened to "class".

@evenstensberg
Copy link
Contributor Author

@GCHQ-Developer-112 PTAL

@evenstensberg
Copy link
Contributor Author

evenstensberg commented Aug 5, 2024

@GCHQ-Developer-112 GCHQ-Developer-112 merged commit 64aa0d4 into mi6:develop Aug 5, 2024
7 checks passed
@evenstensberg evenstensberg deleted the fix/2177 branch August 5, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants