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

Gatsby 5 upgrade #776

Merged
merged 45 commits into from
Jul 18, 2024
Merged

Gatsby 5 upgrade #776

merged 45 commits into from
Jul 18, 2024

Conversation

daneah
Copy link
Member

@daneah daneah commented Jul 18, 2024

This change: (check at least one)

  • Adds a new feature
  • Fixes a bug
  • Improves maintainability
  • Improves documentation
  • Is a release activity

Is this a breaking change? (check one)

  • Yes
  • No

Is the: (complete all)

  • Title of this pull request clear, concise, and indicative of the issue number it addresses, if any?
  • Test suite(s) passing?
  • Code coverage maximal?
  • Changeset added?
  • Component status page up to date?

What does this change address?

How does this change work?

  • Remove use of MDX across the Pharos site, significantly removing and reducing hangups with compatibility between MDX, Gatsby, and Prettier
    • Keep MDX for Storybook intro docs, as that is the only way to generate top-level documentation pages using built-in Storybook behavior. This is rather inconsequential compared to the overall MDX usage we had.

brentswisher and others added 30 commits June 28, 2024 14:56
We still need to fix several components
Previously we used react-live to render the components samples
in the component pages. This is duplication of the functionality storybook
provides and increases the complexity of the site.
Remove the live and render options and updates it to take
a string in instead of a children prop.
@daneah daneah requested a review from a team as a code owner July 18, 2024 01:06
@daneah daneah requested review from sirrah-tam, jialin-he and mtorres3 and removed request for a team July 18, 2024 01:06
Copy link

changeset-bot bot commented Jul 18, 2024

🦋 Changeset detected

Latest commit: 59c60f5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@ithaka/pharos-site Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@daneah daneah changed the title Maintenance/gatsby 5 upgrade Gatsby 5 upgrade Jul 18, 2024
Copy link
Contributor

github-actions bot commented Jul 18, 2024

size-limit report 📦

Path Size
packages/pharos/lib/index.js 64.73 KB (+0.17% 🔺)

@brentswisher
Copy link
Contributor

I was getting an error when building that doesn't show up when running yarn develop, didn't have a ton of time to look into it, something with SSR

@brentswisher
Copy link
Contributor

I'm meeting with David tomorrow to talk about the examples design a bit too, fwiw

@daneah
Copy link
Member Author

daneah commented Jul 18, 2024

@brentswisher @david-corneail let us not let that discussion prevent us from merging this change, as this represents literal months of struggle and frustration—we need to lock in what value we have here and iterate.

The SSR issue is particularly frustrating as it is not of large importance to this project—but we should prioritize fixing it nonetheless mostly again to be able to get past this initial large hurdle. I spent a little bit looking at it and am able to get around it on a page-by-page basis, but it's tedious and doesn't feel like the correct overall approach.

@brentswisher
Copy link
Contributor

So this seems to be the cause of the error 😞

I'm not familiar with focus-trap, I could just add a conditional to it, but I note it hasn't been updated in 3 years, which is 2 years later than what it is forked from. Wondering if there is a better solution for this at today, or a different library we could just switch to that is actively maintained - I'll chat with @sirrah-tam to see

@brentswisher
Copy link
Contributor

After talking with @sirrah-tam I'm gonna see if I can just swap in https://focus-trap.github.io/focus-trap/. We may be able to eliminate it, or at least some of the uses of it, but that would be a little more complicated

@eslawski
Copy link
Contributor

Bit of an aside, but we might want to look at the newly available Popover API at some point in the future which I understand has some amount of focus trapping capabilities out of the box.

@brentswisher
Copy link
Contributor

👍 @eslawski long term I agree, #755 and #347 will likely make the focus-trap less necessary

@brentswisher
Copy link
Contributor

@daneah - @david-corneail and I updated the examples, in some of them (links, breadcrumbs) it wasn't entirely clear they were an example. I also extracted it into a component so we can iterate on it later easier

Before:

Screenshot 2024-07-18 at 11 22 07 AM Screenshot 2024-07-18 at 11 22 55 AM

After:

Screenshot 2024-07-18 at 11 19 23 AM Screenshot 2024-07-18 at 11 26 49 AM

@daneah daneah requested a review from brentswisher July 18, 2024 19:16
@brentswisher brentswisher merged commit f19a406 into develop Jul 18, 2024
7 checks passed
@brentswisher brentswisher deleted the maintenance/gatsby-5-upgrade branch July 18, 2024 21:19
@brentswisher brentswisher mentioned this pull request Jul 19, 2024
11 tasks
@github-actions github-actions bot mentioned this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants