feat: expose containerProps in StudioHeader [FC-0062] #529

rpenido · 2024-09-05T19:46:33Z

Description

This PR adds a new boolean containerProps property to the StudioHeader component. This property will be passed downstream to the Container component in the Header, allowing the user to override the Container props (i.e. changing the max size).

More information

Part of:

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

Depends on:

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

Private ref: FAL-3820

openedx-webhooks · 2024-09-05T19:46:37Z

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 unmaintained.

To get help with finding a technical reviewer, tag the community contributions project manager for this PR in a comment and let them know that your changes are ready for review:

On the right-hand side of the PR, find the Contributions project, click the caret in the top right corner to expand it, and check the "Primary PM" field for the name of your PM.
Find their GitHub handle here.

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.

codecov · 2024-09-05T19:49:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.55%. Comparing base (e44001e) to head (61243f4).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #529      +/-   ##
==========================================
+ Coverage   70.47%   70.55%   +0.08%     
==========================================
  Files          25       25              
  Lines         359      360       +1     
  Branches       94       95       +1     
==========================================
+ Hits          253      254       +1     
  Misses        104      104              
  Partials        2        2

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

rpenido · 2024-09-05T23:51:06Z

src/studio-header/NavDropdownMenu.jsx

@@ -30,7 +30,7 @@ const NavDropdownMenu = ({

 NavDropdownMenu.propTypes = {
  id: PropTypes.string.isRequired,
-  buttonTitle: PropTypes.string.isRequired,
+  buttonTitle: PropTypes.node.isRequired,


Not related to this feature. This is a fly-by fix of a console warning. Let me know if I should create a different PR for this.

[clarification/suggestion] If we make the change here, should we also ensure all other prop type definitions for buttonTitle reflect PropTypes.node, too?

For example, here and here.

Fixed other places here: 06bfcc7

rpenido · 2024-09-05T23:51:55Z

src/studio-header/HeaderBody.jsx

@@ -110,6 +111,7 @@ const HeaderBody = ({
              iconAs={Icon}
              onClick={searchButtonAction}
              aria-label={intl.formatMessage(messages['header.label.search.nav'])}
+              alt={intl.formatMessage(messages['header.label.search.nav'])}


Not related to this feature. This is a fly-by fix of a console warning. Let me know if I should create a different PR for this.

Thanks for fixing these! Fewer warnings is always good.

pomegranited

@rpenido Some comments for you -- and I still need to test this :)

pomegranited · 2024-09-06T06:21:06Z

src/studio-header/HeaderBody.jsx

@@ -110,6 +111,7 @@ const HeaderBody = ({
              iconAs={Icon}
              onClick={searchButtonAction}
              aria-label={intl.formatMessage(messages['header.label.search.nav'])}
+              alt={intl.formatMessage(messages['header.label.search.nav'])}


Thanks for fixing these! Fewer warnings is always good.

pomegranited · 2024-09-06T06:22:26Z

src/studio-header/HeaderBody.jsx

@@ -37,6 +37,7 @@ const HeaderBody = ({
  mainMenuDropdowns,
  outlineLink,
  searchButtonAction,
+  full,


Same comment as https://github.com/openedx/frontend-app-course-authoring/pull/1258/files#diff-1c1e7c5912d6ff798a72899cde884c54211549e22fec1f469c8a5a4a854819e1R20 -- can this be named fullWidth or fullSize instead?

Done: f6673e9

src/studio-header/HeaderBody.jsx

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

adamstankiewicz · 2024-09-10T12:25:36Z

src/studio-header/HeaderBody.jsx

@@ -37,6 +37,7 @@ const HeaderBody = ({
  mainMenuDropdowns,
  outlineLink,
  searchButtonAction,
+  fullWidth,


[consideration] I wonder if it'd make sense to expose a containerProps prop to use as a pass-thru with prop spreading, e.g. <Container size="xl" {...containerProps} />, where it'd stay default to xl but allow consumers reset size back to undefined. This would also give consumers more control to use any of the supported sizes beyond just xl and undefined, e.g. <StudioHeader containerProps={{ size: undefined }} />, <StudioHeader containerProps={{ size: 'lg' }} />. It would also let consumers change other Container props, like fluid per their use cases, e.g. <StudioHeader containerProps={{ fluid: false }} />.

If doing something like containerProps, we'd likely want to merge the containerProps.className with the existing className="px-2.5" on the Container component, e.g. with classNames:

const { containerProps } = props; const { className: containerPropsClassName, ...restContainerProps } = containerProps || {}; <Container size="xl" className={classNames('px-2.5', containerPropsClassName)} {...restContainerProps} > Hello world </Container>

I like the flexibility we would have if we allowed the developer to override the containerProps, but I'm also happy with the abstraction we have here.

Making the ContainerProps explicitly part of the header will tie us to the Container component, and we will have some compatibility issues in the event (very unlikely?) of replacing it in the future.

So I'm divided here, but happy with any path we go.

What are your thoughts @adamstankiewicz?
Do you mind giving us help @pomegranited here?

I like @adamstankiewicz 's idea here and for the footer.. could mean fewer PRs to these repos in future :)

adamstankiewicz · 2024-09-10T12:39:24Z

[inform] It looks like the example MFE application in this repo is broken on the latest version of @openedx/frontend-build, i.e. running npm start results in a blank screen running on localhost:8080. See this PR for a fix to get the example running again, which should make local development within this repo easier by relying on the example app: #531

pomegranited · 2024-09-19T04:48:34Z

src/studio-header/StudioHeader.jsx

@@ -53,6 +54,7 @@ StudioHeader.propTypes = {
  number: PropTypes.string,
  org: PropTypes.string,
  title: PropTypes.string.isRequired,
+  containerProps: HeaderBody.propTypes.containerProps,


containerProps should be optional, and/or populated with a default value below?

Fixed here: e2249b2

pomegranited

👍 This looks and works well, thank you for taking the extra time here @rpenido !

I tested this while testing feat: allow full width content in library authoring [FC-0062] frontend-app-authoring#1258
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

pomegranited · 2024-09-19T22:46:20Z

package.json

@@ -38,7 +38,7 @@
    "@edx/frontend-platform": "8.1.1",
    "@edx/reactifex": "^2.1.1",
    "@openedx/frontend-build": "14.1.2",
-    "@openedx/paragon": "22.7.0",
+    "@openedx/paragon": "22.8.0",


Should this mirror StudioFooter's dependency?

Suggested change

"@openedx/paragon": "22.8.0",

"@openedx/paragon": ">= 21.11.3 < 23.0.0",

Good catch @pomegranited!
The version was wrong in both!
We need paragon 22.8.0 or upper here, so it should be ^22.8.0. We don't need the < 23.0.0 part because the ^ would not allow updates to major versions.

Fixed here: 35c2040

rpenido · 2024-09-20T14:02:24Z

package.json

@@ -38,7 +38,7 @@
    "@edx/frontend-platform": "8.1.1",
    "@edx/reactifex": "^2.1.1",
    "@openedx/frontend-build": "14.1.2",
-    "@openedx/paragon": "22.7.0",
+    "@openedx/paragon": "^22.8.0",


I don't think we need paragon in the devDependencies, but I'm keeping it

rpenido · 2024-09-20T14:04:16Z

Hi @adamstankiewicz! Could you do a review/merge here when you have some time? Thank you!

bradenmacdonald · 2024-09-26T22:18:38Z

@adamstankiewicz we need to ship some features that this is blocking - did you have any further concerns here? If your plate is full I can do a review+merge - but I just want to make sure you don't have any unaddressed concerns.

adamstankiewicz

I had a couple clarifying comments before approving (heads up, in the future: feel free to give me a ping on Slack if you a more urgent review; I tend to miss GitHub notifications 😄)

adamstankiewicz · 2024-09-30T21:58:35Z

package.json

@@ -69,7 +69,8 @@
  },
  "peerDependencies": {
    "@edx/frontend-platform": "^7.0.0 || ^8.0.0",
-    "@openedx/paragon": ">= 21.5.7 < 23.0.0",
+    "@openedx/paragon": "^22.8.0",


Is dropping the previously supported range intentional? As is, this is a breaking change since it drops support for >= 21.5.7.

This was intentional as it was built on top of Paragon 22.8.0, but it will be backward compatible, so I reverted it here: ccee347

But I still think we should update the version here, and I can't see how this could impact someone, as the current package-lock.json is already pointing to 22.8.1.

frontend-component-header/package-lock.json

Lines 3407 to 3408 in e44001e

"node_modules/@openedx/paragon": {

"version": "22.8.1",

Could you help me to understand?

Thank you!

Also, I'm not sure if we should have @openedx/paragon (and some other packages such as react, react-dom, etc..) in the devDependencias, but I think this is an issue for another PR. 😃

The package-lock.json is pointing to 22.8.1 due to @openedx/paragon getting installed as a devDependency, which is intentional. Because this library relies on consumers to provide their own copy of @openedx/paragon (per the peerDependencies), the devDependencies copy of @openedx/paragon is what allows for local development of the library. Without Paragon in the devDependencies, the example app within this repo couldn't run due to the missing installed node_modules/@openedx/paragon.

The devDependencies are not included in the production bundle published to NPM, as confirmed by noting that @openedx/paragon is included only in the npm list --include=dev command, but not npm list --omit=dev as seen below:

❯ npm list --omit=dev --depth=0 @edx/[email protected] /Users/astankiewicz/Desktop/edx/frontend-component-header ├── @fortawesome/[email protected] ├── @fortawesome/[email protected] ├── @fortawesome/[email protected] ├── @fortawesome/[email protected] ├── @fortawesome/[email protected] ├── @openedx/[email protected] ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] └── [email protected] ❯ npm list --include=dev --depth=0 @edx/[email protected] /Users/astankiewicz/Desktop/edx/frontend-component-header ├── @edx/brand@npm:@openedx/[email protected] ├── @edx/[email protected] ├── @edx/[email protected] ├── @edx/[email protected] ├── @fortawesome/[email protected] ├── @fortawesome/[email protected] ├── @fortawesome/[email protected] ├── @fortawesome/[email protected] ├── @fortawesome/[email protected] ├── @openedx/[email protected] ├── @openedx/[email protected] ├── @openedx/[email protected] ├── @testing-library/[email protected] ├── @testing-library/[email protected] ├── @testing-library/[email protected] ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] └── [email protected]

Now I got it! Thank you for the explanation!

adamstankiewicz · 2024-09-30T22:06:09Z

src/studio-header/NavDropdownMenu.jsx

@@ -30,7 +30,7 @@ const NavDropdownMenu = ({

 NavDropdownMenu.propTypes = {
  id: PropTypes.string.isRequired,
-  buttonTitle: PropTypes.string.isRequired,
+  buttonTitle: PropTypes.node.isRequired,


[clarification/suggestion] If we make the change here, should we also ensure all other prop type definitions for buttonTitle reflect PropTypes.node, too?

For example, here and here.

adamstankiewicz

Just one more clarifying comment/suggestion about the classNames dependency.

adamstankiewicz · 2024-10-01T13:05:24Z

package.json

@@ -69,6 +69,7 @@
  },
  "peerDependencies": {
    "@edx/frontend-platform": "^7.0.0 || ^8.0.0",
+    "classnames": "^2.5.1",


I believe we may want to make this a regular dependency under dependencies so consumers don't need to explicitly install classnames themselves. Otherwise, any consuming MFE that doesn't use classnames@^2.5.1 already will need to install it (also technically a breaking change).

classNames is different from packages that should be peerDependencies, which are either large (e.g., @openedx/paragon) and/or should only have a single copy within the consuming application (e.g., @edx/frontend-platform).

Thanks for the explanation! Fixed here: 61243f4

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-allow-full-width-header branch from 484aa8e to ba30f8b Compare September 5, 2024 20:04

rpenido mentioned this pull request Sep 5, 2024

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

Merged

rpenido force-pushed the rpenido/fal-3820-allow-full-width-header branch from ba30f8b to 6ede5ee Compare September 5, 2024 23:12

rpenido commented Sep 5, 2024

View reviewed changes

rpenido changed the title ~~feat: allow full width header~~ feat: allow full width header [FC-0062] Sep 6, 2024

pomegranited suggested changes Sep 6, 2024

View reviewed changes

pomegranited mentioned this pull request Sep 6, 2024

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

Merged

pomegranited approved these changes Sep 10, 2024

View reviewed changes

adamstankiewicz reviewed Sep 10, 2024

View reviewed changes

rpenido marked this pull request as ready for review September 11, 2024 18:29

rpenido mentioned this pull request Sep 17, 2024

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

Merged

rpenido force-pushed the rpenido/fal-3820-allow-full-width-header branch 3 times, most recently from 8741d1b to 192adc7 Compare September 18, 2024 21:51

feat: expose containerProps in StudioHeader

ce107bf

rpenido force-pushed the rpenido/fal-3820-allow-full-width-header branch from 192adc7 to ce107bf Compare September 18, 2024 21:54

rpenido changed the title ~~feat: allow full width header [FC-0062]~~ feat: expose containerProps in StudioHeader [FC-0062] Sep 18, 2024

rpenido requested review from adamstankiewicz and pomegranited September 19, 2024 00:07

pomegranited suggested changes Sep 19, 2024

View reviewed changes

rpenido requested a review from pomegranited September 19, 2024 12:48

rpenido force-pushed the rpenido/fal-3820-allow-full-width-header branch from 41d76cc to 836e8a7 Compare September 19, 2024 20:01

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

rpenido force-pushed the rpenido/fal-3820-allow-full-width-header branch from 836e8a7 to b54ec5a Compare September 19, 2024 20:55

fix: add defaultProps to containerProps

e2249b2

rpenido force-pushed the rpenido/fal-3820-allow-full-width-header branch from b54ec5a to e2249b2 Compare September 19, 2024 21:02

pomegranited approved these changes Sep 19, 2024

View reviewed changes

pomegranited reviewed Sep 19, 2024

View reviewed changes

chore: fix paragon version

35c2040

rpenido force-pushed the rpenido/fal-3820-allow-full-width-header branch from 6658ec5 to 35c2040 Compare September 20, 2024 13:50

Merge branch 'master' into rpenido/fal-3820-allow-full-width-header

d7c69a2

rpenido commented Sep 20, 2024

View reviewed changes

Merge branch 'master' into rpenido/fal-3820-allow-full-width-header

10c3e12

Merge branch 'master' into rpenido/fal-3820-allow-full-width-header

89c949d

adamstankiewicz reviewed Sep 30, 2024

View reviewed changes

rpenido added 2 commits October 1, 2024 09:29

fix: fix types for Dropdown.Button and Dropdown.Item

06bfcc7

chore: fix paragon version

ccee347

adamstankiewicz reviewed Oct 1, 2024

View reviewed changes

chore: change classnames to dependencies

61243f4

adamstankiewicz approved these changes Oct 1, 2024

View reviewed changes

adamstankiewicz merged commit ae5253c into openedx:master Oct 1, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose containerProps in StudioHeader [FC-0062] #529

feat: expose containerProps in StudioHeader [FC-0062] #529

rpenido commented Sep 5, 2024 •

edited

Loading

openedx-webhooks commented Sep 5, 2024 •

edited

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading

rpenido Sep 5, 2024 •

edited

Loading

adamstankiewicz Sep 30, 2024

rpenido Oct 1, 2024

rpenido Sep 5, 2024

pomegranited Sep 6, 2024

pomegranited left a comment

pomegranited Sep 6, 2024

pomegranited Sep 6, 2024

rpenido Sep 6, 2024

pomegranited left a comment

adamstankiewicz Sep 10, 2024

rpenido Sep 10, 2024

pomegranited Sep 18, 2024

adamstankiewicz commented Sep 10, 2024

pomegranited Sep 19, 2024

rpenido Sep 19, 2024 •

edited

Loading

pomegranited left a comment

pomegranited Sep 19, 2024

rpenido Sep 20, 2024 •

edited

Loading

rpenido Sep 20, 2024

rpenido commented Sep 20, 2024 •

edited

Loading

bradenmacdonald commented Sep 26, 2024

adamstankiewicz left a comment

adamstankiewicz Sep 30, 2024

rpenido Oct 1, 2024

rpenido Oct 1, 2024

adamstankiewicz Oct 1, 2024

rpenido Oct 1, 2024

adamstankiewicz Sep 30, 2024

adamstankiewicz left a comment

adamstankiewicz Oct 1, 2024

rpenido Oct 1, 2024

	"@openedx/paragon": "22.8.0",
	"@openedx/paragon": ">= 21.11.3 < 23.0.0",

feat: expose containerProps in StudioHeader [FC-0062] #529

feat: expose containerProps in StudioHeader [FC-0062] #529

Conversation

rpenido commented Sep 5, 2024 • edited Loading

Description

More information

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?

codecov bot commented Sep 5, 2024 • edited Loading

Codecov Report

rpenido Sep 5, 2024 • edited Loading

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

pomegranited left a 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

pomegranited left a 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

adamstankiewicz commented Sep 10, 2024

Choose a reason for hiding this comment

rpenido Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido commented Sep 20, 2024 • edited Loading

bradenmacdonald commented Sep 26, 2024

adamstankiewicz left a 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

adamstankiewicz left a 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 5, 2024 •

edited

Loading

openedx-webhooks commented Sep 5, 2024 •

edited

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading

rpenido Sep 5, 2024 •

edited

Loading

rpenido Sep 19, 2024 •

edited

Loading

rpenido Sep 20, 2024 •

edited

Loading

rpenido commented Sep 20, 2024 •

edited

Loading