Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Jul 14, 2023
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f518e5d
docs: added 'Leave feedback' button to the top of the component page …
PKulkoRaccoonGang Jun 21, 2023
92358d0
refactor: code refactoring
PKulkoRaccoonGang Jun 29, 2023
7a79fa8
refactor: code refactoring
PKulkoRaccoonGang Jul 6, 2023
c6572a5
refactor: replacing Hyperlink with Nav.Link
PKulkoRaccoonGang Jul 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions www/src/components/LeaveFeedback.scss
View file
Open in desktop

This file was deleted.

44 changes: 36 additions & 8 deletions www/src/components/LeaveFeedback.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,57 @@
import React from 'react';
import React, { AnchorHTMLAttributes } from 'react';
import PropTypes from 'prop-types';
import { Nav } from '~paragon-react';
import { Button, Hyperlink } from '~paragon-react';
import { useLocation } from '@reach/router';

function LeaveFeedback(props) {
export interface LeaveFeedbackProps extends Partial<AnchorHTMLAttributes<HTMLAnchorElement>> {
isLink?: boolean;
}

function LeaveFeedback({ isLink, ...props }: LeaveFeedbackProps) {
const location = useLocation();
const FEEDBACK_URL = `https://github.com/openedx/paragon/issues/new?title=%5Bparagon-openedx.netlify.app%5D%20Feedback%20(on%20${location.pathname})&amp;labels=docs&template=feedback_template.md`;
const leaveFeedbackLinkTitle = 'Leave feedback';

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

if (isLink) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
  )
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return (
<Hyperlink
className="muted-link nav-link"
destination={FEEDBACK_URL}
showLaunchIcon={false}
onClick={handleLinkFeedbackClick}
target="_blank"
{...props}
>
{leaveFeedbackLinkTitle}
</Hyperlink>
);
}

return (
<Nav.Link onClick={handleLinkFeedbackClick} href={FEEDBACK_URL} target="_blank" {...props}>
Leave feedback
</Nav.Link>
<Button
as={Hyperlink}
destination={FEEDBACK_URL}
size="sm"
variant="outline-primary"
onClick={handleLinkFeedbackClick}
target="_blank"
{...props}
>
{leaveFeedbackLinkTitle}
</Button>
);
}

LeaveFeedback.propTypes = {
className: PropTypes.string,
isLink: PropTypes.bool,
};

LeaveFeedback.defaultProps = {
className: 'muted-link',
isLink: false,
};

export default LeaveFeedback;
33 changes: 23 additions & 10 deletions www/src/components/PageLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
Row,
Col,
Sticky,
Hyperlink,
useMediaQuery,
breakpoints,
} from '~paragon-react';
Expand Down Expand Up @@ -83,7 +84,7 @@ function Layout({
{children}
<Container size="md">
<hr />
<LeaveFeedback className="pgn__docs-page-feedback-link" />
<LeaveFeedback className="mb-5" />
</Container>
</Col>
<Col
Expand Down Expand Up @@ -114,28 +115,40 @@ function Layout({
</Link>
</Nav.Item>
<Nav.Item>
<Nav.Link
className="muted-link"
href="https://github.com/openedx/.github/blob/master/CODE_OF_CONDUCT.md"
<Hyperlink
className="muted-link nav-link"
destination="https://github.com/openedx/.github/blob/master/CODE_OF_CONDUCT.md"
showLaunchIcon={false}
target="_blank"
>
Code of Conduct
</Nav.Link>
</Hyperlink>
</Nav.Item>
<Nav.Item>
<Nav.Link className="muted-link" href="https://open.edx.org/">
<Hyperlink
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Contributor Author

@PKulkoRaccoonGang PKulkoRaccoonGang Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I agree with you
Thanks 👍

className="muted-link nav-link"
destination="https://open.edx.org/"
showLaunchIcon={false}
target="_blank"
>
Open edX
</Nav.Link>
</Hyperlink>
</Nav.Item>
<Nav.Item>
<LeaveFeedback />
<LeaveFeedback isLink />
</Nav.Item>
<div className="flex-grow-1" />
<a href="https://www.netlify.com">
<Hyperlink
className="muted-link nav-link"
destination="https://www.netlify.com"
showLaunchIcon={false}
target="_blank"
>
<img
src="https://www.netlify.com/img/global/badges/netlify-light.svg"
alt="Deploys by Netlify"
/>
</a>
</Hyperlink>
</Nav>
</Container>
</div>
Expand Down
1 change: 0 additions & 1 deletion www/src/scss/base.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
@import "../components/LinkedHeading";
@import "../components/Menu";
@import "../components/doc-elements";
@import "../components/LeaveFeedback";
@import "../components/Settings";
@import "../components/Toc";
@import "../components/header/Header";
Expand Down
6 changes: 5 additions & 1 deletion www/src/templates/component-page-template.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import Layout from '../components/PageLayout';
import SEO from '../components/SEO';
import LinkedHeading from '../components/LinkedHeading';
import ComponentsUsage from '../components/insights/ComponentsUsage';
import LeaveFeedback from '../components/LeaveFeedback';

export interface IPageTemplate {
data: {
Expand Down Expand Up @@ -136,7 +137,10 @@ export default function PageTemplate({
<p className="small mb-0">{mdx.frontmatter.notes}</p>
</Alert>
)}
<h1 className="mb-4">{mdx.frontmatter.title}</h1>
<div className="d-flex justify-content-between align-items-start">
<h1 className="mb-4">{mdx.frontmatter.title}</h1>
<LeaveFeedback />
</div>
<MDXProvider components={shortcodes}>
<MDXRenderer>{mdx.body}</MDXRenderer>
</MDXProvider>
Expand Down