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

✨(frontend) replace grommet Heading #2410

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

AntoLC
Copy link
Contributor

@AntoLC AntoLC commented Sep 12, 2023

Purpose

Replace grommet Heading with a mix of Cunningham and our own wrapper.

Proposal

  • create own Heading wrapper
  • replace occurrences of grommet Heading in the codespace

Review

Extra check here: https://github.com/openfun/marsha/pull/2410/files#diff-338a7653e6c76d240c205006ef794dd9aa28e209ebd7d202042946df64d597e6

@AntoLC AntoLC self-assigned this Sep 12, 2023
@AntoLC AntoLC force-pushed the feature/anthony/replace-heading-grommet branch 4 times, most recently from fc321e2 to 480ca92 Compare September 15, 2023 10:52
@AntoLC AntoLC marked this pull request as ready for review September 15, 2023 11:45
Copy link
Contributor

@kernicPanel kernicPanel left a comment

Choose a reason for hiding this comment

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

I still have to make it run, but LGTM for now 👍

@@ -44,21 +44,21 @@ const TextBannerBox = styled(Box)`
`;

interface LayoutProps {
isFullLayout: boolean;
$isFullLayout: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

FMI, why adding a $ here ?

Copy link
Member

Choose a reason for hiding this comment

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

the jquery nostalgia

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the props are propagating to the component Heading then the DOM, but we don't want isFullLayout to propagate because it is used only with the styled component HeadingBanner, so to block the propagation you can add a dollar in front of your variable (https://styled-components.com/docs/basics#passed-props).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh OK, thanks !

UploadsView is not used anywhere in the app, so it can be deleted.
In order to do the transition to the cunningham design system,
we replace the grommet Heading component with our own Heading
component that uses the cunningham design system.
@AntoLC AntoLC force-pushed the feature/anthony/replace-heading-grommet branch from 480ca92 to ebc7e43 Compare September 18, 2023 08:12
@AntoLC AntoLC merged commit 20efe58 into master Sep 18, 2023
34 checks passed
@AntoLC AntoLC deleted the feature/anthony/replace-heading-grommet branch September 18, 2023 13:26
lunika added a commit that referenced this pull request Oct 2, 2023
Added

- Send an email when a live stream is ready to be converted to VOD
- Send an email when a live stream is converted to VOD
- Send an email when a live stream is about to be deleted

Changed

- Replace grommet Heading (#2410)
- Upgrade to python 3.11
- Upgrade channels and channels-redis to version 4

Fixed

- force bbb user_id uniqueness in join url
lunika added a commit that referenced this pull request Oct 2, 2023
Added

- Send an email when a live stream is ready to be converted to VOD
- Send an email when a live stream is converted to VOD
- Send an email when a live stream is about to be deleted

Changed

- Replace grommet Heading (#2410)
- Upgrade to python 3.11
- Upgrade channels and channels-redis to version 4

Fixed

- force bbb user_id uniqueness in join url
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants