-
Notifications
You must be signed in to change notification settings - Fork 117
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
feat: expose containerProps in StudioHeader [FC-0062] #529
base: master
Are you sure you want to change the base?
feat: expose containerProps in StudioHeader [FC-0062] #529
Conversation
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 approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf 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:
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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
484aa8e
to
ba30f8b
Compare
ba30f8b
to
6ede5ee
Compare
@@ -30,7 +30,7 @@ const NavDropdownMenu = ({ | |||
|
|||
NavDropdownMenu.propTypes = { | |||
id: PropTypes.string.isRequired, | |||
buttonTitle: PropTypes.string.isRequired, | |||
buttonTitle: PropTypes.node.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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'])} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing these! Fewer warnings is always good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpenido Some comments for you -- and I still need to test this :)
@@ -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'])} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing these! Fewer warnings is always good.
src/studio-header/HeaderBody.jsx
Outdated
@@ -37,6 +37,7 @@ const HeaderBody = ({ | |||
mainMenuDropdowns, | |||
outlineLink, | |||
searchButtonAction, | |||
full, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as https://github.com/openedx/frontend-app-course-authoring/pull/1258/files#diff-1c1e7c5912d6ff798a72899cde884c54211549e22fec1f469c8a5a4a854819e1R20 -- can this be named fullWidth
or fullSize
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: f6673e9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
- 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 issuesN/A -
Includes documentationN/A -
User-facing strings are extracted for translationN/A
src/studio-header/HeaderBody.jsx
Outdated
@@ -37,6 +37,7 @@ const HeaderBody = ({ | |||
mainMenuDropdowns, | |||
outlineLink, | |||
searchButtonAction, | |||
fullWidth, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like @adamstankiewicz 's idea here and for the footer.. could mean fewer PRs to these repos in future :)
[inform] It looks like the example MFE application in this repo is broken on the latest version of |
8741d1b
to
192adc7
Compare
192adc7
to
ce107bf
Compare
@@ -53,6 +54,7 @@ StudioHeader.propTypes = { | |||
number: PropTypes.string, | |||
org: PropTypes.string, | |||
title: PropTypes.string.isRequired, | |||
containerProps: HeaderBody.propTypes.containerProps, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
containerProps
should be optional, and/or populated with a default value below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here: e2249b2
41d76cc
to
836e8a7
Compare
836e8a7
to
b54ec5a
Compare
b54ec5a
to
e2249b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 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 issuesN/A -
Includes documentationN/A -
User-facing strings are extracted for translationN/A
package.json
Outdated
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this mirror StudioFooter's dependency?
"@openedx/paragon": "22.8.0", | |
"@openedx/paragon": ">= 21.11.3 < 23.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
6658ec5
to
35c2040
Compare
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need paragon in the devDependencies
, but I'm keeping it
Hi @adamstankiewicz! Could you do a review/merge here when you have some time? Thank you! |
@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. |
Description
This PR adds a new boolean
containerProps
property to theStudioHeader
component. This property will be passed downstream to theContainer
component in the Header, allowing the user to override theContainer
props (i.e. changing the max size).More information
Part of:
Depends on:
Private ref: FAL-3820