-
Notifications
You must be signed in to change notification settings - Fork 0
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
Upgrade quince #6
Conversation
package.json
Outdated
@@ -53,6 +53,7 @@ | |||
"react-redux": "7.2.9", | |||
"react-router": "5.2.1", | |||
"react-router-dom": "5.3.0", | |||
"react-query": "^3.39.3", |
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.
fix indentation
@import "~@edx/brand/paragon/fonts.scss"; | ||
@import "~@edx/brand/paragon/variables.scss"; | ||
@import "~@edx/paragon/scss/core/core.scss"; | ||
@import "~@edx/brand/paragon/overrides.scss"; | ||
@import '~@edx/brand/paragon/fonts.scss'; | ||
@import '~@edx/brand/paragon/variables.scss'; | ||
@import '~@edx/paragon/scss/core/core.scss'; | ||
@import '~@edx/brand/paragon/overrides.scss'; | ||
|
||
$fa-font-path: "~font-awesome/fonts"; | ||
@import "~font-awesome/scss/font-awesome"; | ||
$fa-font-path: '~font-awesome/fonts'; | ||
@import '~font-awesome/scss/font-awesome'; |
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.
Why make changes that are not necessary? This will make transition and blaming hard for later.
if they're using a linting config, kindly set your editor so follow those rules.
<Header courseOrg={org} courseNumber={courseNumber} courseTitle={courseTitle} /> | ||
<Header /> |
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.
Can't we just have our Header component show any text we pass to it? so we don't need to delete the data showing in the header?
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.
We don’t have a specific header for this MFE; we use a unique header for all MFEs. Here, they use the learning header, which has differences compared to the main header.
src/head/Head.jsx
Outdated
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.
And also, we have a header component for a reason, to not repeat ourselves. if we needed a change, we just updated the header component and it should be available everywhere. why do we have a Header component here?
2d9b48b
to
3ae18c7
Compare
6d52f9f
to
fe687f7
Compare
Description
Include a description of your changes here, along with a link to any relevant Jira tickets and/or GitHub issues.
How Has This Been Tested?
Please describe in detail how you tested your changes.
Screenshots/sandbox (optional):
Include a link to the sandbox for design changes or screenshot for before and after. Remove this section if it's not applicable.
Merge Checklist
Post-merge Checklist