feat: migrates Container to TypeScript; Container without max-width on docs site [FC-0062] #3216

rpenido · 2024-09-05T15:08:38Z

Description

This PR allows the size property of the Container to be undefined, removing the maximum width in this case

Deploy Preview

https://deploy-preview-3216--paragon-openedx.netlify.app/components/container/

More information

Part of:

Adjustments to tile responsiveness when sidebar is opened frontend-app-authoring#1221

Merge Checklist

If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
Does your change adhere to the documented style conventions?
Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
Were your changes tested in the example app?
Is there adequate test coverage for your changes?
Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please request an a11y review for the PR in the #wg-paragon Open edX Slack channel.

Post-merge Checklist

Verify your changes were released to NPM at the expected version.
If you'd like, share your contribution in #show-and-tell.
🎉 🙌 Celebrate! Thanks for your contribution.

Private ref: FAL-3820

openedx-webhooks · 2024-09-05T15:08:42Z

Thanks for the pull request, @rpenido!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/paragon-working-group. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

netlify · 2024-09-05T15:08:55Z

✅ Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`7a03d83`
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/66df513b5bad7a0008eb3ffe
😎 Deploy Preview	https://deploy-preview-3216--paragon-openedx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-09-05T15:22:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.27%. Comparing base (4b63a24) to head (7a03d83).
Report is 6 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3216      +/-   ##
==========================================
+ Coverage   93.25%   93.27%   +0.01%     
==========================================
  Files         249      249              
  Lines        4388     4401      +13     
  Branches     1037     1037              
==========================================
+ Hits         4092     4105      +13     
  Misses        290      290              
  Partials        6        6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pomegranited

@rpenido Some comments for you -- I still need to test this too.

And in future, could you separate out converting from JSX -> TSX into a separate commit? That would help me see what's changed :)

pomegranited · 2024-09-06T06:01:59Z

src/Container/README.md

@@ -19,6 +19,10 @@ The base container to contain, pad, and center content in the viewport. This com
 ```jsx live
 <div style={{ overflowX: 'auto' }}>
    <div style={{ width: '1500px', border: 'solid 3px red' }}>
+        <Container size="full" className="bg-danger-300 my-4">
+          The content in this container don't have a max width


nit:

Suggested change

The content in this container don't have a max width

The content in this container doesn't have a max width

Fixed: a55d28b

pomegranited · 2024-09-06T06:10:02Z

src/Container/index.tsx

+    bsPrefix="container"
+    fluid


nit: Do we need these lines when these properties are overridden by the prop defaults below?

I'm more concerned with the fluid one, since I'm unsure how overriding these boolean values works in Typescript, and don't want to break anyone else's UI.

Suggested change

bsPrefix="container"

fluid

You're right. We don't need this.
Removed here: a55d28b

As information only, because the spread {...props} was after these props, the spread would override it. The boolean fluid is a syntax sugar for fluid={true}. This is the transpiled JS code:

pomegranited · 2024-09-06T06:15:29Z

src/Container/Container.test.tsx

+  it('does not add a size class when size is not specified', () => {
+    const { container } = render(<Container>Content</Container>);


I'd rather use an explicit size="full" than assume that omitting the size means "full width".. Omitting a size should make the container fit its contents, just like a <div> without a max-width.
I think the bootstrap class mw-100 does this?

I agree with you and tried this approach first.

The problem is that the current component allows the size props to be omitted, and it behaves like "full width". You can check it here, removing the size from one example: https://paragon-openedx.netlify.app/components/container/

We have many containers in our code base without size that probably rely on this, so I don't think we can change that.
So I pushed back the size={full} option.

We can add both options, but I think the ambiguity will not help us here.

I tried to update the API docs here: 47cbcb1

The problem is that the current component allows the size props to be omitted, and it behaves like "full width". You can check it here, removing the size from one example: https://paragon-openedx.netlify.app/components/container/

Passing in size="" gives us the fullscreen behaviour you're adding here.. so do we need this change?

Edit: ah yes, this is what Adam said 😄

adamstankiewicz · 2024-09-08T16:58:08Z

Thanks for the contribution! Just as a heads up, I believe the primary change here is the TypeScript migration and adding the docs site example for Container without a max-width. The current implementation of Container supports the intended behavior today when omitting the size prop: Playground Example.

The first instance of Container is the above example renders with max-width: none;, when size is omitted or an invalid size is passed:

adamstankiewicz · 2024-09-08T17:00:54Z

src/Container/index.tsx

@@ -0,0 +1,64 @@
+/* eslint-disable react/require-default-props */


[optional] With a recent update to @edx/eslint-config, we might be able to upgrade to v4.2.0 in this repo and remove the need for default params in this file without disabling the ESLint rule 👀 openedx/eslint-config#156

While this repo is currently using v3 of @edx/eslint-config, the v4 breaking changes don't seem to apply to this repo at the moment, per the release notes, so the upgrade should be fairly trivial:

Drops support for eslint ^6.8.0; @edx/eslint-config must now be used with at least eslint versions ^7.32.0 or ^8.2.0.

@adamstankiewicz I tried to update but got the following error on linting (which prevents the commit).
Did you already experienced this error before?

> @openedx/[email protected] lint > npm run stylelint && eslint --ext .js --ext .jsx --ext .ts --ext .tsx . && npm run lint --workspaces --if-present > @openedx/[email protected] stylelint > stylelint "src/**/*.scss" "scss/**/*.scss" "www/src/**/*.scss" --config .stylelintrc.json Oops! Something went wrong! :( ESLint: 8.18.0 Error: Error while loading rule '@typescript-eslint/naming-convention': You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser. Occurred while linting /home/rpenido/Projects/opencraft/paragon/.eslintrc.js ...

Hmm, I have not. Thanks for flagging. I am able to reproduce the issue on my end as well. Unless you want to investigate the error further, feel free to defer on the upgrade and continue with your existing solution using default props and the disabled ESLint rule. If you do defer on the investigation/resolution, do you mind filing a new Paragon issue to document the error?

Issue created: #3230

adamstankiewicz · 2024-09-08T17:05:18Z

src/Container/index.tsx

+  size?: ContainerSize;
+}
+
+type ContainerType = ComponentWithAsProp<'div', ContainerProps> & { Deprecated?: any };


Is the Deprecated property necessary here? Container doesn't have a deprecated sub-component (e.g., Container.Deprecated) as Button.Deprecated does.

No, it's not needed. Thank you for catching this.
Fixed here: 641dd15

adamstankiewicz · 2024-09-08T17:21:13Z

src/Container/index.tsx

+
+type ContainerType = ComponentWithAsProp<'div', ContainerProps> & { Deprecated?: any };
+
+const Container: ContainerType = React.forwardRef<HTMLDivElement, ContainerProps>(({


Could Container be used with a non-div element, e.g. with as="span"? I wonder if this should be more generic, e.g. HTMLElement or even Element.

Yes! I changed it to Element as our property of interest (className) exists.
Here: 7e0fb5c

adamstankiewicz · 2024-09-08T17:22:18Z

src/Container/index.tsx

+  /** Fill all available space at any breakpoint */
+  fluid: PropTypes.bool,
+  /** Set the maximum width for the container. Omiting the prop will remove the max-width */
+  size: PropTypes.oneOf(['xs', 'sm', 'md', 'lg', 'xl', undefined]),


I believe the undefined is implied by the lack of .isRequired on the prop type definition?

Agreed. It was there to show in the docs, but it isn't necessary with the docstring change.
Fixed: 7a03d83

adamstankiewicz · 2024-09-08T17:23:16Z

www/src/context/SettingsContext.tsx

@@ -12,7 +12,7 @@ export interface IDefaultValue {
    theme?: string,
    direction?: string,
    language?: string,
-    containerWidth?: string,
+    containerWidth?: ContainerSize,


Thanks for adding the ContainerSize type here, too! 🙌

rpenido · 2024-09-09T19:50:42Z

Just as a heads up, I believe the primary change here is the TypeScript migration and adding the docs site example for Container without a max-width.

Absolutely. Do you think I should change anything in the PR to make it clear?

adamstankiewicz · 2024-09-09T19:56:41Z

Absolutely. Do you think I should change anything in the PR to make it clear?

I might just recommend amending your PR title / commit message to more closely reflect the TypeScript migration and the additional example on the docs site, e.g.

feat: migrates Container to TypeScript; Container without max-width on docs site

Edit: typically we squash+merge when merging PRs, so this refactored commit message can actually be provided at the time the PR merges without you needing to amend/squash the commits yourself! Perhaps just modifying the PR title for now would suffice 😄

adamstankiewicz · 2024-09-09T20:43:26Z

@rpenido When this merges, to ensure it gets published to NPM via semantic-release, the commit type prefix will be feat.

semantic-release, by default (as configured), only triggers NPM releases for fix, feat, and perf. Given your contribution is exposing new TypeScript types to consumers, I want to be sure merging triggers a new release feat :)

rpenido · 2024-09-09T20:50:47Z

Thank you, @adamstankiewicz! I updated the PR title again.
When you and @pomegranited think this is good for merge, just ping me, and I can squash it (if needed) to ensure we have the correct commit message.

adamstankiewicz · 2024-09-09T20:56:05Z

LGTM! If either you or @pomegranited don't have the ability to merge, let me know and I'll happily merge it, too :)

pomegranited · 2024-09-10T01:54:45Z

Thanks @adamstankiewicz could you merge? We don't have access.

pomegranited

👍

I tested this while testing feat: allow full width content in library authoring [FC-0062] frontend-app-authoring#1258 (review)
I read through the code
~~I checked for accessibility issues~~ N/A
~~Includes documentation~~ N/A
~~User-facing strings are extracted for translation~~ N/A

openedx-semantic-release-bot · 2024-09-10T12:17:10Z

🎉 This PR is included in version 22.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 5, 2024

rpenido force-pushed the rpenido/fal-3820-add-full-width-container-option branch 2 times, most recently from 9996590 to 98e8bdb Compare September 5, 2024 18:04

This was referenced Sep 5, 2024

feat: expose containerProps in StudioHeader [FC-0062] openedx/frontend-component-header#529

Open

feat: expose containerProps in StudioFooter [FC-0062] openedx/frontend-component-footer#463

Open

rpenido force-pushed the rpenido/fal-3820-add-full-width-container-option branch from 98e8bdb to 0dfd9d8 Compare September 5, 2024 20:27

rpenido mentioned this pull request Sep 5, 2024

feat: allow full width content in library authoring [FC-0062] openedx/frontend-app-authoring#1258

Draft

feat: allow Container render without a max width

4a206ae

rpenido force-pushed the rpenido/fal-3820-add-full-width-container-option branch from 0dfd9d8 to 4a206ae Compare September 5, 2024 22:48

rpenido changed the title ~~feat: allow Container render without a max width~~ feat: allow Container render without a max width [FC-0062] Sep 6, 2024

pomegranited suggested changes Sep 6, 2024

View reviewed changes

rpenido marked this pull request as ready for review September 6, 2024 14:25

rpenido added 2 commits September 6, 2024 11:29

docs: update size property api docs

47cbcb1

fix: props override and docs

a55d28b

adamstankiewicz reviewed Sep 8, 2024

View reviewed changes

rpenido added 3 commits September 9, 2024 16:40

fix: remove deprecated from ContainerType

641dd15

fix: change forwardRef from HTMLDivElement to Element

7e0fb5c

fix: remove unecessary undefined prop value

7a03d83

rpenido changed the title ~~feat: allow Container render without a max width [FC-0062]~~ refactor: migrates Container to TypeScript; Container without max-width on docs site [FC-0062] Sep 9, 2024

rpenido changed the title ~~refactor: migrates Container to TypeScript; Container without max-width on docs site [FC-0062]~~ feat: migrates Container to TypeScript; Container without max-width on docs site [FC-0062] Sep 9, 2024

adamstankiewicz approved these changes Sep 9, 2024

View reviewed changes

pomegranited approved these changes Sep 10, 2024

View reviewed changes

adamstankiewicz merged commit 65bb39b into openedx:master Sep 10, 2024
10 checks passed

openedx-semantic-release-bot added the released label Sep 10, 2024

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Sep 10, 2024

rpenido deleted the rpenido/fal-3820-add-full-width-container-option branch September 10, 2024 19:13

rpenido mentioned this pull request Sep 17, 2024

fix: responsiveness on library authoring sidebar [FC-0062] openedx/frontend-app-authoring#1293

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrates Container to TypeScript; Container without max-width on docs site [FC-0062] #3216

feat: migrates Container to TypeScript; Container without max-width on docs site [FC-0062] #3216

rpenido commented Sep 5, 2024 •

edited

Loading

openedx-webhooks commented Sep 5, 2024 •

edited

Loading

netlify bot commented Sep 5, 2024 •

edited

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading

pomegranited left a comment

pomegranited Sep 6, 2024

rpenido Sep 6, 2024 •

edited

Loading

pomegranited Sep 6, 2024

rpenido Sep 6, 2024 •

edited

Loading

pomegranited Sep 6, 2024

rpenido Sep 6, 2024 •

edited

Loading

rpenido Sep 6, 2024 •

edited

Loading

pomegranited Sep 10, 2024 •

edited

Loading

adamstankiewicz commented Sep 8, 2024

adamstankiewicz Sep 8, 2024

rpenido Sep 9, 2024

adamstankiewicz Sep 9, 2024

rpenido Sep 10, 2024

adamstankiewicz Sep 8, 2024

rpenido Sep 9, 2024

adamstankiewicz Sep 8, 2024

rpenido Sep 9, 2024

adamstankiewicz Sep 8, 2024

rpenido Sep 9, 2024

adamstankiewicz Sep 8, 2024

rpenido commented Sep 9, 2024

adamstankiewicz commented Sep 9, 2024 •

edited

Loading

adamstankiewicz commented Sep 9, 2024 •

edited

Loading

rpenido commented Sep 9, 2024

adamstankiewicz commented Sep 9, 2024

pomegranited commented Sep 10, 2024

pomegranited left a comment

openedx-semantic-release-bot commented Sep 10, 2024

	The content in this container don't have a max width
	The content in this container doesn't have a max width

		it('does not add a size class when size is not specified', () => {
		const { container } = render(<Container>Content</Container>);

		@@ -0,0 +1,64 @@
		/* eslint-disable react/require-default-props */


		type ContainerType = ComponentWithAsProp<'div', ContainerProps> & { Deprecated?: any };

		const Container: ContainerType = React.forwardRef<HTMLDivElement, ContainerProps>(({

feat: migrates Container to TypeScript; Container without max-width on docs site [FC-0062] #3216

feat: migrates Container to TypeScript; Container without max-width on docs site [FC-0062] #3216

Conversation

rpenido commented Sep 5, 2024 • edited Loading

Description

Deploy Preview

More information

Merge Checklist

Post-merge Checklist

openedx-webhooks commented Sep 5, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

netlify bot commented Sep 5, 2024 • edited Loading

✅ Deploy Preview for paragon-openedx ready!

codecov bot commented Sep 5, 2024 • edited Loading

Codecov Report

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

rpenido Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

pomegranited Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

adamstankiewicz commented Sep 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido commented Sep 9, 2024

adamstankiewicz commented Sep 9, 2024 • edited Loading

adamstankiewicz commented Sep 9, 2024 • edited Loading

rpenido commented Sep 9, 2024

adamstankiewicz commented Sep 9, 2024

pomegranited commented Sep 10, 2024

pomegranited left a comment

Choose a reason for hiding this comment

openedx-semantic-release-bot commented Sep 10, 2024

rpenido commented Sep 5, 2024 •

edited

Loading

openedx-webhooks commented Sep 5, 2024 •

edited

Loading

netlify bot commented Sep 5, 2024 •

edited

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading

rpenido Sep 6, 2024 •

edited

Loading

rpenido Sep 6, 2024 •

edited

Loading

rpenido Sep 6, 2024 •

edited

Loading

rpenido Sep 6, 2024 •

edited

Loading

pomegranited Sep 10, 2024 •

edited

Loading

adamstankiewicz commented Sep 9, 2024 •

edited

Loading

adamstankiewicz commented Sep 9, 2024 •

edited

Loading