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.

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

feat(app): Add resources page to more section #1631

Merged
merged 3 commits into from
Jun 7, 2018
Merged

Conversation

Kadee80
Copy link
Contributor

@Kadee80 Kadee80 commented Jun 6, 2018

overview

This PR closes #1607 - adds resources page to more section of the app.

screen shot 2018-06-06 at 11 44 25 am

changelog

  • group AppSettings and Resources pages in /More, refactor routes
  • add KnowledgeCard and LibraryCard to resources page

review requests

Standards review. Please click around and make sure [more] NavButton is correctly highlighted. and nothing whitescreens.

@Kadee80 Kadee80 requested review from pantslakz, mcous and IanLondon June 6, 2018 17:02
Copy link

@pantslakz pantslakz left a comment

Choose a reason for hiding this comment

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

YAAAAAAHHHHH!!!!!!!

@codecov
Copy link

codecov bot commented Jun 6, 2018

Codecov Report

Merging #1631 into edge will decrease coverage by 0.21%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##            edge    #1631      +/-   ##
=========================================
- Coverage   34.4%   34.18%   -0.22%     
=========================================
  Files        338      343       +5     
  Lines       5526     5561      +35     
=========================================
  Hits        1901     1901              
- Misses      3625     3660      +35
Impacted Files Coverage Δ
app/src/components/nav-bar/NavButton.js 0% <ø> (ø) ⬆️
app/src/components/App.js 0% <ø> (ø) ⬆️
app/src/components/MenuPanel/index.js 0% <ø> (ø) ⬆️
app/src/pages/More/Resources.js 0% <0%> (ø)
app/src/pages/More/index.js 0% <0%> (ø)
app/src/components/Resources/index.js 0% <0%> (ø)
app/src/components/Resources/ResourceCard.js 0% <0%> (ø)
app/src/pages/More/AppSettings.js 0% <0%> (ø)
... and 3 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 886f2e9...4042483. Read the comment docs.

<p>Download a protocol to run on your robot</p>
<OutlineButton
Component="a"
href="http://protocols.opentrons.com/"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do https by default


const TITLE = 'Protocol Library'

export default function LibraryCard () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need separate components for KnowledgeCard and LibraryCard? Seems like we could get away with a ExternalResourceCard that takes children or a message prop and an external 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.

Fair enough. Will change with https comment below

const {match: {path}} = props
return (
<Switch>
<Redirect exact from={path} to={'/menu/app'} />
Copy link
Contributor

Choose a reason for hiding this comment

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

to prop should use path to build the route so we can change the top level path (/menu) in only one place

<Redirect exact from={path} to={`${path}/app`} />

<Switch>
<Redirect exact from={path} to={'/menu/app'} />
<Route
path={'/menu/app'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also use path to build route

path={`${path}/app`}

Given the redirect above, it might be worth doing...

const appPath = `${path}/app`

...up top for the Redirect's to and the Route's path

component={AppSettings}
/>
<Route
path={'/menu/resources'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also use path to build route

path={`${path}/resources`}

Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

🍹

@pantslakz
Copy link

@Kadee80 some small edits:

Knowledge base should say:
"Support Articles" and should link to: support.opentrons.com/ot-2

@Kadee80 Kadee80 merged commit 443afc0 into edge Jun 7, 2018
@Kadee80 Kadee80 deleted the app_add-resources-page branch June 7, 2018 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add 'Resources' page to 'More' panel
3 participants