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: Remove built in wrapper for React SSR react components #535

Merged
merged 24 commits into from
Jun 7, 2024

Conversation

ethanWallace
Copy link
Collaborator

Summary | Résumé

Remove built in GcdsWrapper from react components provided from @cdssnc/gcds-components-react-ssr to allow more flexible rendering. The GcdsWrapper will now have to be imported along with the components. Also solves issue with using className on the react components.

Usage

Example of how to use the GcdsWrapper with the GC Design System components

import {
  GcdsHeader,
  GcdsSearch
} from "@cdssnc/gcds-components-react-ssr";
import { GcdsWrapper } from '@cdssnc/gcds-components-react-ssr/client'
import React, { FC } from "react";

export const Header: FC = () => {
  return (
    <GcdsWrapper>
      <GcdsHeader langHref={langToggleHref} skipToHref="#">
        <GcdsSearch
          action="/searchresults"
          method="post"
          name="searchbox"
          searchId="searchbox"
          placeholder="Search this site"
          slot="search"
        ></GcdsSearch>
      </GcdsHeader>
    </GcdsWrapper>
  );
};

renovate bot and others added 4 commits May 21, 2024 08:05
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* chore: GCDS Components Release

* chore: update lerna and changelog

* Fix typo

---------

Co-authored-by: sre-read-write[bot] <92993749+sre-read-write[bot]@users.noreply.github.com>
Co-authored-by: Ethan Wallace <ethan.wallace.91@gmail.com>
@ethanWallace ethanWallace requested a review from daine May 22, 2024 13:32
melaniebmn and others added 11 commits May 22, 2024 14:09
…536)

refactor: update input component to increase input width calculation + remove max-length
…INES.md (#538)

* Update and rename CONTRIBUTING.md to CONTRIBUTION GUIDELINES.md

Updating contribution guidelines content in both English and French

* Rename CONTRIBUTION GUIDELINES.md to CONTRIBUTING

* Rename CONTRIBUTING to CONTRIBUTING.md
fix: misaligned of FR theme and topic menu button
…equest and bug report) (#533)

* docs: add fr translation for issue templates (contribution)

* chore: update translated template for bug reports

* docs: update formatting

* chore: add feature request issue template french translations

* removing extra quotation mark

Co-authored-by: Élise Cossette <45772213+EliseKa@users.noreply.github.com>

* removing extra quotation mark

Co-authored-by: Élise Cossette <45772213+EliseKa@users.noreply.github.com>

---------

Co-authored-by: Élise Cossette <45772213+EliseKa@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* chore: GCDS Components Release

* Update CHANGELOG.md

* chore: update lerna version + changelog

---------

Co-authored-by: sre-read-write[bot] <92993749+sre-read-write[bot]@users.noreply.github.com>
Co-authored-by: Melanie Boeckmann <melanie.bockmann@gmail.com>
* chore: synced local '.github/workflows/s3-backup.yml' with remote 'tools/sre_file_sync/s3-backup.yml'

* chore: synced local '.github/workflows/export_github_data.yml' with remote 'tools/sre_file_sync/export_github_data.yml'

* chore: synced local '.github/workflows/ossf-scorecard.yml' with remote 'tools/sre_file_sync/ossf-scorecard.yml'

---------

Co-authored-by: sre-read-write[bot] <92993749+sre-read-write[bot]@users.noreply.github.com>
fix: card property typos in Storybook
@ethanWallace
Copy link
Collaborator Author

@daine Finally figured out why the build was not working in our workflows, should be good to review now.

melaniebmn and others added 3 commits May 28, 2024 11:34
…ew (#540)

* chore(storybook): add custom copy code button to storybook code preview

* wip

* damn you button haha

* add min-width to copy button

* revert button copy after 1.5 seconds
* chore(deps): update dependency @angular/core to v10 [security]

* chore: Update angular dependencies

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Ethan Wallace <ethan.wallace.91@gmail.com>
@daine
Copy link
Collaborator

daine commented May 28, 2024

Thanks for this work @ethanWallace 🌟

Just a few things I've run into:

  1. At the moment I'm not sure what's the difference between wrapping the GcdsWrapper around my gcds-components. Running in dev mode doesn't show any difference, it runs just fine without it? I could have missed something. 🤔
  2. Removing our automatic wrapping certainly helps with adding non-gcds components inside gcds components. I can now create custom components for example <TopNav> to encapsulate all my top navigation code and use that within `GcdsHeader. Awesome! 👏
  3. 🥺 Still getting that issue with className, but only with typecript and npm run build (linting and type validation). It runs fine on dev mode but it won't build. The IDE also complains with the same error message:
./app/components/layout/Header.tsx:81:43
Type error: Type '{ children: Element[]; href: string; slot: string; className: string; }' is not assignable to type 'IntrinsicAttributes & Partial<EnumsToStringLiterals<GcdsLink> & { slot: string; } & GlobalEventHandlers & { ...; }> & { ...; } & RefAttributes<...>'.
  Property 'className' does not exist on type 'IntrinsicAttributes & Partial<EnumsToStringLiterals<GcdsLink> & { slot: string; } & GlobalEventHandlers & { ...; }> & { ...; } & RefAttributes<...>'.
> 81 |       <GcdsLink href="/" slot="signature" className="d-flex align-items-center link-default">
     |

@ethanWallace
Copy link
Collaborator Author

@daine the GcdsWrapper is what allows the components to be rendered server side. Without it the components will only render client side.

@ethanWallace
Copy link
Collaborator Author

@daine I think it should be good for review now

Copy link
Collaborator

@daine daine left a comment

Choose a reason for hiding this comment

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

So I checked a few things and I think this PR is good to merge in!

  • I'm able to use my own component within a Gcds component (i.e. custom PhaseBanner inside GcdsHeader
  • I can use the className with a Gcds component now
  • No longer getting the attribute error for boolean props (i.e. current on nav links)
  • It's not part of this, but in my nextjs integration example PR I've now updated all the form components to use GcdsWrapper and it works well when I click on "view source" to show the rendered HTML with declarative shadow dom

LGTM!

@ethanWallace ethanWallace merged commit 390eadd into gcds-components-ssr Jun 7, 2024
2 checks passed
@ethanWallace ethanWallace deleted the fix/react-ssr-wrapper branch June 7, 2024 12:22
@daine daine mentioned this pull request Jul 24, 2024
8 tasks
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.

4 participants