fix: Hard-coded link in Header component changed by LMS_BASE_URL env var #186

ChrisChV · 2021-05-25T21:42:07Z

index.jsx file of Header component updated with change the hard-coded link of https://www.edx.org with a component property.
index.jsx file of root project updated. Added the environment variable LMS_BASE_URL to the parameters of the Header component

Issue: openedx/wg-build-test-release#55

What changed?

This updates the index.jsx file of the Header component with change the hard-coded link with the component property logoDestination.
This updates the index.jsx file of root project with add the LMS_BASE_URL env var as logoDestination in Header component.

Developer Checklist

Test suites passing
Documentation and test plan updated, if applicable
Received code-owner approving review
Bumped version number package.json

Testing Instructions

If you start your local development server for gradebook and you load a course, when you click in the header logo, then it should redirect to the LMS dashboard.

Reviewer Checklist

Collectively, these should be completed by reviewers of this PR:

I've done a visual code review
I've tested the new functionality

FYI: @edx/masters-devs-gta

On index.js file of the Header component, currenly there is a hard-coded link of "https://www.edx.org". I added a new enviroment variable LOGO_DESTINATION to use it in this Header. I have also added a mechanism to use the LMS_BASE_URL environment variable if the LOGO_DESTINATION environment variable is not found or is null.

openedx-webhooks · 2021-05-25T21:42:11Z

Thanks for the pull request, @ChrisChV! I've created OSPR-5806 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:

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.

sarina · 2021-05-26T13:09:00Z

@davidjoy didn't you fix this already?

natabene · 2021-05-26T14:03:36Z

@ChrisChV Thank you for the contribution! Once you have signed our Contributor Agreement and it has been processed, you will receive a confirmation via email from our Legal team. We then will be able to review this and all your future code contributions.

essiocolom · 2021-05-26T17:49:40Z

@natabene we already signed the Entity Contributor Agreement and sent it to legal as we are going to be contributing as Bitmaker and @ChrisChV is part of the organization 👍 By the way, we are still waiting for them to confirm everything is okay

natabene · 2021-05-26T17:53:06Z

@ChrisChV Thank you for an update! They will process and get back to you soon.

ChrisChV · 2021-05-26T21:39:32Z

Hi @natabene. We have already been authorized as a bitmaker to contribute

davidjoy · 2021-05-27T01:37:09Z

Looking around at the shared MFE header, I think we should set the logo destination to point at the dashboard for consistency:

${config.LMS_BASE_URL}/dashboard

In which case we don't need to add a new environment variable specific to gradebook.

See shared header implementation here: https://github.com/edx/frontend-component-header/blob/3726792df260e72bcf9be6cd3bc921b880255604/src/Header.jsx#L82

ChrisChV · 2021-05-27T01:51:32Z

Looking around at the shared MFE header, I think we should set the logo destination to point at the dashboard for consistency:

${config.LMS_BASE_URL}/dashboard

In which case we don't need to add a new environment variable specific to gradebook.

See shared header implementation here: https://github.com/edx/frontend-component-header/blob/3726792df260e72bcf9be6cd3bc921b880255604/src/Header.jsx#L82

@davidjoy In this case it is better to place the logo destination value in the Header as I have done now? Or pass that value through the Header props from the index file, as being done in the link you shared?

natabene · 2021-05-27T16:07:57Z

@ChrisChV Thank you for joining the Open edX community! We have added your user to our records and can now review your code going forward. @davidjoy Would you like to review?

natabene · 2021-05-27T20:11:18Z

@mattcarter FYI this PR coming.

`index.jsx` file of Header component updated with change the hard-coded link of https://www.edx.org with a component property. `index.jsx` file of root project updated. Added the environment variable LMS BASE_URL to the parameters of the Header Component

ChrisChV · 2021-05-28T01:18:06Z

@davidjoy Please review the new changes, I added a new logic to solve this using what you commented and without creating a new env var

natabene · 2021-05-28T15:33:29Z

@mattcarter Do you want to have a look now?

jansenk · 2021-05-28T15:48:29Z

@mattcarter @natabene I've got some spare cycles, I can review this if you'd like

mattcarter · 2021-05-28T16:51:24Z

@jansenk, that would be great. Please review and engage with @davidjoy

jansenk

Looks good! Small style suggestion

jansenk · 2021-05-28T19:48:07Z

src/components/Header/index.jsx

    return (
      <div className="mb-3">
        <header className="d-flex justify-content-center align-items-center p-3 border-bottom-blue">
-          <Hyperlink destination="https://www.edx.org">
+          <Hyperlink {...logoProps}>


Nit: the variable declarations seem unnecessary, you could just have this be <Hyperlink destination={this.props.logoDestination}>

I think this line should just read:

<Hyperlink destination={`${getConfig().LMS_BASE_URL}/dashboard`}>

It's not really a configurable thing/there doesn't seem to be much value in passing it down from above.

davidjoy

I think we can simplify this and just put the URL in the hyperlink. The index.jsx file doesn't really need to know about the URL/this code isn't reused with different URLs.

davidjoy · 2021-06-03T14:15:39Z

src/components/Header/index.jsx

    return (
      <div className="mb-3">
        <header className="d-flex justify-content-center align-items-center p-3 border-bottom-blue">
-          <Hyperlink destination="https://www.edx.org">
+          <Hyperlink {...logoProps}>


I think this line should just read:

<Hyperlink destination={`${getConfig().LMS_BASE_URL}/dashboard`}>

It's not really a configurable thing/there doesn't seem to be much value in passing it down from above.

ChrisChV · 2021-06-05T00:27:45Z

@davidjoy It's done

natabene · 2021-06-14T16:21:53Z

@davidjoy @jansenk If this is good to merge, let's merge.

openedx-webhooks · 2021-06-14T17:51:23Z

@ChrisChV 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added community manager review open-source-contribution PR author is not from Axim or 2U labels May 25, 2021

ChrisChV mentioned this pull request May 25, 2021

The gradebook MFE header includes a hardcoded link to edx.org openedx/wg-build-test-release#55

Closed

ChrisChV changed the title ~~feat: LOGO_DESTINATION env var added for use it on Header component~~ feat: LOGO_DESTINATION env var added for use in Header component May 25, 2021

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed community manager review labels May 27, 2021

ChrisChV changed the title ~~feat: LOGO_DESTINATION env var added for use in Header component~~ fix: Hard-coded link in Header component changed by a env var May 28, 2021

ChrisChV changed the title ~~fix: Hard-coded link in Header component changed by a env var~~ fix: Hard-coded link in Header component changed by LMS_BASE_URL env var May 28, 2021

openedx-webhooks added engineering review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels May 28, 2021

fix: lint errors in previous commits

5356eef

ChrisChV marked this pull request as ready for review May 31, 2021 22:54

jansenk approved these changes Jun 1, 2021

View reviewed changes

openedx-webhooks removed the engineering review label Jun 1, 2021

openedx-webhooks added the changes requested label Jun 1, 2021

davidjoy suggested changes Jun 3, 2021

View reviewed changes

style: change structure of the solution

5b10369

davidjoy approved these changes Jun 11, 2021

View reviewed changes

openedx-webhooks added engineering review and removed changes requested labels Jun 14, 2021

mattcarter merged commit 02c154e into openedx:master Jun 14, 2021

openedx-webhooks added merged and removed engineering review labels Jun 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Hard-coded link in Header component changed by LMS_BASE_URL env var #186

fix: Hard-coded link in Header component changed by LMS_BASE_URL env var #186

ChrisChV commented May 25, 2021 •

edited by mattcarter

openedx-webhooks commented May 25, 2021 •

edited

sarina commented May 26, 2021

natabene commented May 26, 2021

essiocolom commented May 26, 2021

natabene commented May 26, 2021

ChrisChV commented May 26, 2021

davidjoy commented May 27, 2021

ChrisChV commented May 27, 2021

natabene commented May 27, 2021

natabene commented May 27, 2021

ChrisChV commented May 28, 2021

natabene commented May 28, 2021

jansenk commented May 28, 2021

mattcarter commented May 28, 2021

jansenk left a comment

jansenk May 28, 2021

davidjoy Jun 3, 2021

davidjoy left a comment

davidjoy Jun 3, 2021

ChrisChV commented Jun 5, 2021

natabene commented Jun 14, 2021

openedx-webhooks commented Jun 14, 2021

fix: Hard-coded link in Header component changed by LMS_BASE_URL env var #186

fix: Hard-coded link in Header component changed by LMS_BASE_URL env var #186

Conversation

ChrisChV commented May 25, 2021 • edited by mattcarter

openedx-webhooks commented May 25, 2021 • edited

sarina commented May 26, 2021

natabene commented May 26, 2021

essiocolom commented May 26, 2021

natabene commented May 26, 2021

ChrisChV commented May 26, 2021

davidjoy commented May 27, 2021

ChrisChV commented May 27, 2021

natabene commented May 27, 2021

natabene commented May 27, 2021

ChrisChV commented May 28, 2021

natabene commented May 28, 2021

jansenk commented May 28, 2021

mattcarter commented May 28, 2021

jansenk left a comment

Choose a reason for hiding this comment

jansenk May 28, 2021

Choose a reason for hiding this comment

davidjoy Jun 3, 2021

Choose a reason for hiding this comment

davidjoy left a comment

Choose a reason for hiding this comment

davidjoy Jun 3, 2021

Choose a reason for hiding this comment

ChrisChV commented Jun 5, 2021

natabene commented Jun 14, 2021

openedx-webhooks commented Jun 14, 2021

ChrisChV commented May 25, 2021 •

edited by mattcarter

openedx-webhooks commented May 25, 2021 •

edited