[BD-46] docs: added 'Leave feedback' button to the top of the component page #2395

PKulkoRaccoonGang · 2023-06-21T20:18:13Z

Description

Issue: #2373

Deploy Preview

Include a direct link to your changes in this PR's deploy preview here (e.g., a specific component page).

Merge Checklist

If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
Does your change adhere to the documented style conventions?
Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
Were your changes tested in the example app?
Is there adequate test coverage for your changes?
Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

Verify your changes were released to NPM at the expected version.
If you'd like, share your contribution in #show-and-tell.
🎉 🙌 Celebrate! Thanks for your contribution.

openedx-webhooks · 2023-06-21T20:18:18Z

Thanks for the pull request, @PKulkoRaccoonGang!

When this pull request is ready, tag your edX technical lead.

netlify · 2023-06-21T20:18:22Z

✅ Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`c6572a5`
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/64b1233d34fba9000870aef9
😎 Deploy Preview	https://deploy-preview-2395--paragon-openedx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2023-06-22T06:51:10Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (b24807d) 91.38% compared to head (7a79fa8) 91.38%.

❗ Current head 7a79fa8 differs from pull request most recent head c6572a5. Consider uploading reports for the commit c6572a5 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2395   +/-   ##
=======================================
  Coverage   91.38%   91.38%           
=======================================
  Files         234      234           
  Lines        4157     4157           
  Branches     1001     1001           
=======================================
  Hits         3799     3799           
  Misses        351      351           
  Partials        7        7

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

www/src/components/LeaveFeedback.tsx

…template

adamstankiewicz · 2023-06-23T12:05:08Z

www/src/templates/component-page-template.tsx

-        <h1 className="mb-4">{mdx.frontmatter.title}</h1>
+        <div className="d-flex justify-content-between">
+          <h1 className="mb-4">{mdx.frontmatter.title}</h1>
+          <LeaveFeedback as={Hyperlink} />


If we're using Hyperlink here to get it to have the external launch icon, we may also want to include it on the other in-page "Leave feedback" link towards the bottom.

Likewise, we could also use as={Hyperlink} in the docs site footer such that we get the correct rel="noopener noreferrer" attribute for security. That said, given none of the other external links in the footer include an external launch icon, we can use Hyperlink's showLaunchIcon={false} to hide the icon for the footer only.

adamstankiewicz · 2023-06-23T12:25:15Z

www/src/templates/component-page-template.tsx

-        <h1 className="mb-4">{mdx.frontmatter.title}</h1>
+        <div className="d-flex justify-content-between">
+          <h1 className="mb-4">{mdx.frontmatter.title}</h1>
+          <LeaveFeedback as={Hyperlink} />


Per the issue, I believe the intent for "Leave feedback" now is to be a small Button (variant="outline-primary") that acts as a Hyperlink here in component-page-template and below the page content (though not the footer).

adamstankiewicz · 2023-06-23T12:28:24Z

www/src/templates/component-page-template.tsx

-        <h1 className="mb-4">{mdx.frontmatter.title}</h1>
+        <div className="d-flex justify-content-between">
+          <h1 className="mb-4">{mdx.frontmatter.title}</h1>
+          <LeaveFeedback as={Hyperlink} />


It looks like this defaults to using Nav.Link styles by default, even though most uses of LeaveFeedback don't actually intend to use those styles. I wonder if the use of LeaveFeedback in the footer should be the exception that gets the Nav.Link styles rather than having to override that in 2 of the 3 places the LeaveFeedback component is used.

Is there a way to perhaps make this component a bit more flexible so the differing styles in the various use cases don't conflict?

Thanks for the review! I replaced the top link and the bottom link with buttons, I also changed the implementation of links in the site footer. It seems to me that all links that lead to third-party resources should have rel="noopener noreferrer" and target="_blank ".

viktorrusakov · 2023-07-13T18:58:47Z

www/src/components/PageLayout.tsx

          </Nav.Item>
          <Nav.Item>
-            <Nav.Link className="muted-link" href="https://open.edx.org/">
+            <Hyperlink


I don't think you need to replace implementation in all these links. We should always use Nav.Link component when we want to render a link inside a Nav, that's exactly why this component exists.

Instead of using Hyperlink and manually using nav-link SCSS class (which should ideally be internal to Nav.Link component) you can achieve the same result with

<Nav.Link className="muted-link" href="https://open.edx.org/" target="_blank" rel="noopener noreferrer" > Open edX </Nav.Link>

@viktorrusakov thanks for the review 👍

The transition from Nav.Link to HyperLink was due to my assumptions about the semantic meaning of the Nav.Link component and its purpose. After researching the Bootstrap page for the Nav.Link component, it seemed to me the right decision to use the HyperLink component for navigation links, because the Nav.Link component should probably be used to navigate the internal links of the application. It is for this reason that the Nav.Link component does not have the corresponding attributes that HyperLink does.

An interesting article about our question

It still just renders an a tag whether you use Nav.Link or HyperLink, but Nav.Link ensures that the link looks correctly inside a nav while for HyperLink you need to add additional class (which is internal to Nav.Link) to make it work.

I agree with your semantics reasoning (that nav probably shouldn't have external links in it), but your solution does not solve the underlying problem - in the DOM it still renders a nav element with bunch of external links inside it. If we really care about this, then we need to remove nav altogether and render HyperLink's inside a ul I think.

Ok, I agree with you
Thanks 👍

viktorrusakov · 2023-07-13T19:03:38Z

www/src/components/LeaveFeedback.tsx


  const handleLinkFeedbackClick = () => {
    global.analytics.track('openedx.paragon.docs.leave_feedback.clicked');
  };

+  if (isLink) {


I think it also make more sense to leave Nav.Link implementation here since we only use it inside Nav component, e.g.

if (isNavLink) { return ( <Nav.Link onClick={handleLinkFeedbackClick} href={FEEDBACK_URL} target="_blank" rel="noopener noreferrer" {...props} > {leaveFeedbackLinkTitle} </Nav.Link> ) }

adamstankiewicz

LGTM!

openedx-webhooks · 2023-07-14T12:46:20Z

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

edx-semantic-release · 2023-07-14T13:09:39Z

🎉 This PR is included in version 20.45.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

…penedx#2395)

openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Jun 21, 2023

PKulkoRaccoonGang mentioned this pull request Jun 21, 2023

[paragon-openedx.netlify.app] Feedback (on /components/alert/) #2373

Closed

PKulkoRaccoonGang self-assigned this Jun 21, 2023

PKulkoRaccoonGang requested review from monteri and adamstankiewicz June 21, 2023 20:21

PKulkoRaccoonGang force-pushed the Peter_Kulko/leave-feedback-on-the-top branch 2 times, most recently from b97acef to 279e7b7 Compare June 22, 2023 06:29

monteri reviewed Jun 22, 2023

View reviewed changes

www/src/components/LeaveFeedback.tsx Outdated Show resolved Hide resolved

docs: added 'Leave feedback' button to the top of the component page …

f518e5d

…template

PKulkoRaccoonGang force-pushed the Peter_Kulko/leave-feedback-on-the-top branch from 279e7b7 to f518e5d Compare June 22, 2023 08:22

adamstankiewicz reviewed Jun 23, 2023

View reviewed changes

PKulkoRaccoonGang force-pushed the Peter_Kulko/leave-feedback-on-the-top branch 3 times, most recently from bcbca21 to e873c97 Compare June 29, 2023 08:43

refactor: code refactoring

92358d0

PKulkoRaccoonGang force-pushed the Peter_Kulko/leave-feedback-on-the-top branch from e873c97 to 92358d0 Compare June 29, 2023 08:45

PKulkoRaccoonGang requested a review from adamstankiewicz June 29, 2023 11:05

refactor: code refactoring

7a79fa8

viktorrusakov reviewed Jul 13, 2023

View reviewed changes

PKulkoRaccoonGang requested a review from viktorrusakov July 14, 2023 10:15

refactor: replacing Hyperlink with Nav.Link

c6572a5

PKulkoRaccoonGang force-pushed the Peter_Kulko/leave-feedback-on-the-top branch from c7f72f2 to c6572a5 Compare July 14, 2023 10:28

adamstankiewicz approved these changes Jul 14, 2023

View reviewed changes

adamstankiewicz merged commit 00dd998 into openedx:master Jul 14, 2023
7 checks passed

edx-semantic-release added the released label Jul 14, 2023

monteri added the raccoon-gang label Jul 17, 2023

pedromartello pushed a commit to hammerlabs-net/paragon that referenced this pull request Aug 10, 2023

docs: added 'Leave feedback' button to the top of the component page (o…

71e36c8

…penedx#2395)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BD-46] docs: added 'Leave feedback' button to the top of the component page #2395

[BD-46] docs: added 'Leave feedback' button to the top of the component page #2395

PKulkoRaccoonGang commented Jun 21, 2023 •

edited

Loading

openedx-webhooks commented Jun 21, 2023

netlify bot commented Jun 21, 2023 •

edited

Loading

codecov bot commented Jun 22, 2023 •

edited

Loading

adamstankiewicz Jun 23, 2023

adamstankiewicz Jun 23, 2023

adamstankiewicz Jun 23, 2023

PKulkoRaccoonGang Jun 29, 2023

viktorrusakov Jul 13, 2023

PKulkoRaccoonGang Jul 13, 2023 •

edited

Loading

viktorrusakov Jul 14, 2023

PKulkoRaccoonGang Jul 14, 2023

viktorrusakov Jul 13, 2023

PKulkoRaccoonGang Jul 14, 2023

adamstankiewicz left a comment

openedx-webhooks commented Jul 14, 2023

edx-semantic-release commented Jul 14, 2023

[BD-46] docs: added 'Leave feedback' button to the top of the component page #2395

[BD-46] docs: added 'Leave feedback' button to the top of the component page #2395

Conversation

PKulkoRaccoonGang commented Jun 21, 2023 • edited Loading

Description

Deploy Preview

Merge Checklist

Post-merge Checklist

openedx-webhooks commented Jun 21, 2023

netlify bot commented Jun 21, 2023 • edited Loading

✅ Deploy Preview for paragon-openedx ready!

codecov bot commented Jun 22, 2023 • edited Loading

Codecov Report

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

PKulkoRaccoonGang Jul 13, 2023 • 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

adamstankiewicz left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Jul 14, 2023

edx-semantic-release commented Jul 14, 2023

PKulkoRaccoonGang commented Jun 21, 2023 •

edited

Loading

netlify bot commented Jun 21, 2023 •

edited

Loading

codecov bot commented Jun 22, 2023 •

edited

Loading

PKulkoRaccoonGang Jul 13, 2023 •

edited

Loading