feat: introduced LearningHeader for frontend-app-learning #133

asadiqbal08 · 2021-10-28T08:28:46Z

Earlier Review References of @davidjoy

As per some suggestion by David, We need to refactor the code in EdX Learning MFE to utilize the header from frontend-component-header instead of course-header

.

openedx-webhooks · 2021-10-28T08:28:49Z

Thanks for the pull request, @asadiqbal08! I've created OSPR-6184 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

src/learning-header/Header.jsx

src/generic/messages.js

davidjoy

Two small changes, but this looks like what we're after. Would you mind just running through the messages really quick and making sure they're all in use in here?

asadiqbal08 · 2021-11-01T12:20:20Z

@davidjoy anything else need to address here or good to go ?

davidjoy · 2021-11-15T21:09:21Z

Ugh, it still seems to have some package-lock.json problems. Try running npm ci locally to see the error - looks like package-lock.json doesn't like some font-awesome versions. Does running npm i produce any changes to package-lock.json? If not, we may need to just re-generate package-lock.json by deleting it and running npm i, but it'd be preferable not to if we can avoid it.

Otherwise this looks good to go and as soon as we get that check to pass, we can merge.

davidjoy

It looks like there are issues with package-lock.json - see my prior comment above.

asadiqbal08 · 2021-11-22T10:08:43Z

Yes, I forgot to push updated package-lock.json, Now I am not getting any error locally. let see over the CI pipeline here.

asadiqbal08 · 2021-11-25T11:15:19Z

@davidjoy I updated the PR to fix issues that causing build failure in the CI pipeline. I make sure by re running the Github action on local devstack and also fixed the corresponding test failure issues. 🤞🤞

asadiqbal08 · 2021-12-01T11:07:17Z

src/Header.jsx

    {
      type: 'item',
      href: config.LOGOUT_URL,
      content: intl.formatMessage(messages['header.user.menu.logout']),
    },
  ];

+  // Users should only see Order History if have a ORDER_HISTORY_URL define in the environment.
+  if (config.ORDER_HISTORY_URL) {
+    userMenu.splice(-1, 0, orderHistoryItem);


@davidjoy FYI, I kept the OrderHistory conditional on basis of ORDER_HISTORY_URL

asadiqbal08 · 2021-12-01T11:08:59Z

src/learning-header/AuthenticatedUserDropdown.jsx

+          <Dropdown.Item href={`${getConfig().LMS_BASE_URL}/account/settings`}>
+            {intl.formatMessage(messages.account)}
+          </Dropdown.Item>
+          { getConfig().ORDER_HISTORY_URL && (


codecov-commenter · 2021-12-02T15:43:53Z

Codecov Report

Merging #133 (6e31803) into master (56963c2) will increase coverage by 58.91%.
The diff coverage is 81.48%.

@@             Coverage Diff             @@
##           master     #133       +/-   ##
===========================================
+ Coverage        0   58.91%   +58.91%     
===========================================
  Files           0       14       +14     
  Lines           0      258      +258     
  Branches        0       60       +60     
===========================================
+ Hits            0      152      +152     
- Misses          0      105      +105     
- Partials        0        1        +1

Impacted Files	Coverage Δ
src/i18n/index.js	`100.00% <ø> (ø)`
src/learning-header/AnonymousUserMenu.jsx	`33.33% <33.33%> (ø)`
src/Header.jsx	`89.47% <66.66%> (ø)`
src/learning-header/AuthenticatedUserDropdown.jsx	`83.33% <83.33%> (ø)`
src/learning-header/LearningHeader.jsx	`92.30% <92.30%> (ø)`
src/generic/messages.js	`100.00% <100.00%> (ø)`
src/learning-header/messages.js	`100.00% <100.00%> (ø)`
src/MobileHeader.jsx	`86.11% <0.00%> (ø)`
src/Logo.jsx	`66.66% <0.00%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56963c2...6e31803. Read the comment docs.

openedx-webhooks · 2021-12-02T19:17:24Z

@asadiqbal08 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

asadiqbal08 requested a review from a team October 28, 2021 08:28

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Oct 28, 2021

asadiqbal08 mentioned this pull request Oct 28, 2021

Introduced a new header as LearningHeader from Frontend Learning MFE. mitodl/frontend-component-header-mitol#1

Closed

openedx-webhooks added engineering review and removed needs triage labels Oct 28, 2021

davidjoy reviewed Oct 28, 2021

View reviewed changes

src/learning-header/Header.jsx Outdated Show resolved Hide resolved

davidjoy reviewed Oct 28, 2021

View reviewed changes

src/generic/messages.js Outdated Show resolved Hide resolved

davidjoy suggested changes Oct 28, 2021

View reviewed changes

asadiqbal08 requested a review from davidjoy October 29, 2021 08:13

This was referenced Oct 29, 2021

WIP - asadiqbal08/Refactore course header from Learning MFE openedx/frontend-app-learning#714

Closed

asadiqbal08/Removed course header from Learning MFE openedx/frontend-app-learning#715

Merged

davidjoy changed the title ~~asadiqbal08/Introduced Learning Course Header in MFE.~~ feat: introduced LearningHeader for frontend-app-learning Nov 1, 2021

asadiqbal08 force-pushed the asadiqbal08/learning-header branch 4 times, most recently from de21631 to 5e7358d Compare November 8, 2021 09:32

davidjoy suggested changes Nov 19, 2021

View reviewed changes

pdpinch mentioned this pull request Nov 22, 2021

Support for the Gradebook MFE openedx/wg-build-test-release#29

Closed

asadiqbal08 force-pushed the asadiqbal08/learning-header branch 2 times, most recently from 4670174 to 736739d Compare November 25, 2021 11:13

asadiqbal08 requested a review from davidjoy November 25, 2021 11:13

asadiqbal08 force-pushed the asadiqbal08/learning-header branch from 789075c to 6deacc0 Compare November 30, 2021 08:28

asadiqbal08 commented Dec 1, 2021

View reviewed changes

feat: Introduced Learning Course Header in Header MFE.

6e31803

asadiqbal08 force-pushed the asadiqbal08/learning-header branch from 93eb8be to 6e31803 Compare December 1, 2021 11:08

asadiqbal08 commented Dec 1, 2021

View reviewed changes

davidjoy merged commit 2db0fd5 into openedx:master Dec 2, 2021

openedx-webhooks added merged and removed engineering review labels Dec 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduced LearningHeader for frontend-app-learning #133

feat: introduced LearningHeader for frontend-app-learning #133

asadiqbal08 commented Oct 28, 2021 •

edited

Loading

openedx-webhooks commented Oct 28, 2021

davidjoy left a comment

asadiqbal08 commented Nov 1, 2021

davidjoy commented Nov 15, 2021

davidjoy left a comment

asadiqbal08 commented Nov 22, 2021 •

edited

Loading

asadiqbal08 commented Nov 25, 2021

asadiqbal08 Dec 1, 2021

asadiqbal08 Dec 1, 2021

codecov-commenter commented Dec 2, 2021

openedx-webhooks commented Dec 2, 2021

feat: introduced LearningHeader for frontend-app-learning #133

feat: introduced LearningHeader for frontend-app-learning #133

Conversation

asadiqbal08 commented Oct 28, 2021 • edited Loading

Earlier Review References of @davidjoy

openedx-webhooks commented Oct 28, 2021

davidjoy left a comment

Choose a reason for hiding this comment

asadiqbal08 commented Nov 1, 2021

davidjoy commented Nov 15, 2021

davidjoy left a comment

Choose a reason for hiding this comment

asadiqbal08 commented Nov 22, 2021 • edited Loading

asadiqbal08 commented Nov 25, 2021

asadiqbal08 Dec 1, 2021

Choose a reason for hiding this comment

asadiqbal08 Dec 1, 2021

Choose a reason for hiding this comment

codecov-commenter commented Dec 2, 2021

Codecov Report

openedx-webhooks commented Dec 2, 2021

asadiqbal08 commented Oct 28, 2021 •

edited

Loading

asadiqbal08 commented Nov 22, 2021 •

edited

Loading