[Banner] Refactor to a functional component and fix its id server/client mismatch

[Tom-Bonnike]

WHY are these changes introduced?

We’re getting server/client id mismatches on the Admin dashboard on pages that use Banners. This is because the current way of generating unique ids is not foolproof.

WHAT is this pull request doing?

The first commit is refactoring Banner to a functional component since we can’t use hooks in class components.
The second commit fixes the id server/client mismatch by using the useUniqueId hook instead of the Banner’s custom uniqueID() util.

How to 🎩

We can’t 🎩 the bug fix on the Playground since it’s not server-side rendered.

Here‘s how I tested it:

• Go to a page on your local web that renders a Banner; I used /admin/settings/payments. Notice how there’s a warning in the console that looks like this one:

• Checkout the fix-mismatch-banner-ids branch on polaris-react
• Run yarn build then yarn build-consumer web
• Restart your web server then refresh the page on /admin/settings/payments, notice how there’s no more warning in the console

I think relying on automated testing for the rest is OK here as I’m not really changing any behaviour/styles, just implementation details: the unit tests & visual regression tests are passing.
Let me know if you still want me to go through the regular 🎩 checklist.

[ghost]

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

[github-actions]

🟢 No significant changes to src/**/*.tsx were detected.

[Tom-Bonnike]

Moving from a class to a functional component is probs not worth an entry in Code quality?

[Tom-Bonnike]

I tried it and it seems to still work great! You can paste this in the Playground to 🎩:

import React from 'react';

import {Page, Banner} from '../src';

export function Playground() {
  const bannerRef = React.useRef();

  React.useEffect(() => {
    bannerRef.current?.focus();
  }, []);

  return (
    <Page title="Playground">
      <Banner ref={bannerRef} title="Some banner title" />
    </Page>
  );
}

[BPScott]

Yay thanks for doing this! I've not had time to review yet, but shall try and get to it early next week

[chloerice]

Thanks for contributing, @Tom-Bonnike, the code looks great! I tried to tophat in web, but in order to get dev up working I had to reset railgun so I didn't have any banners visible because my data is so bare. The tophat in Chroma looks solid with and without the new design lang enabled 🚢

[BPScott]

Note to self: When telling people you'll review something at the start of the week remember that the first half of your week is full of meetings.

Thanks for doing this and sorry for the delay!

I've dropped a few comments inline. The important one that needs to be fixed is adding the role=status to non-critical/non-warning banners as you pointed out. Turns out we did that before but it got lost in the refactoring :)

[Tom-Bonnike]

Thanks for the reviews! Applied your comments @BPScott, good catch on that aria role 🙈

[Tom-Bonnike]

I moved that up with the other icon/role tests and added the role assertions to those as well.

[BPScott]

Thanks for being patient while I was on holiday these past few days. Fantastic work, looks all great to me!

Polaris convention is that you merge your own PRs once you're happy with them (the polaris-team being the ones that merge would imply we're some kind of gatekeepers which we absolutely don't want to encourage - everyone should feel like they own this codebase) so when you're happy

good catch on that aria role

There's a very real chance I would have totally missed it if not for your initial pondering around that, so thank you for pointing it out!

[Tom-Bonnike]

Thanks! Cool. I’ll merge as soon as CI passes. I’m guessing y’all are still responsible for releasing the new versions?