Card View in React and Patternfly #4
Conversation
frontend/src/App.css
Outdated
| @@ -0,0 +1,38 @@ | |||
| .App { | |||
There was a problem hiding this comment.
I don't think you're using a lot of this css. Get rid of any and all you don't need
There was a problem hiding this comment.
Yes sir, data cleansing will be performed all over
frontend/src/App.js
Outdated
| //import accessibleStyles from '@patternfly/react-styles/css/utilities/Accessibility/accessibility'; | ||
| //import spacingStyles from '@patternfly/react-styles/css/utilities/Spacing/spacing'; | ||
| //import { css } from '@patternfly/react-styles'; | ||
| import imgOpenShift from './openShift.svg' |
There was a problem hiding this comment.
Let's move all these images into an images folder. I think we need to embrace a directory structure that will scale as your app grows. For now I propose:
src
├── App.css
├── App.js
├── App.test.js
├── images
│ ├── airflow-2.png
│ ... more images ...
│ └── spark.png
├── index.css
├── index.js
├── Launcher
│ ├── components
│ │ ├── index.js
│ │ └── OdhAppCard.js
│ ├── index.js
│ └── Launcher.js
There was a problem hiding this comment.
Oh, come up with consistent naming/capitalization for the image files, too. Names like kubeflowPng.png could be better
There was a problem hiding this comment.
Ohh yes this image isn't used - it be organized well now onwards with proper naming
frontend/src/App.js
Outdated
| skipToContent={PageSkipToContent} | ||
| mainContainerId={pageId} | ||
| > | ||
| <PageSection variant={PageSectionVariants.light}> |
There was a problem hiding this comment.
Let's not put all this functionality in a single App component, let's break it down. Start by creating a component called Launcher or whatever you think fits in it's own folder and use a <Launcher/> component here. It should return this PageSection as it's result. Later we'll use these sections for the routed pages.
There was a problem hiding this comment.
We may change this later, but I tend to prefer grouping by page/route. https://reactjs.org/docs/faq-structure.html#grouping-by-features-or-routes
In this case we're building the Launcher page or feature.
There was a problem hiding this comment.
Noted, got your point - it makes sense
frontend/src/App.js
Outdated
| > | ||
| <PageSection variant={PageSectionVariants.light}> | ||
| <Grid hasGutter> | ||
| <GridItem span={3} style={{ borderStyle:"solid", borderColor:"#00264d" ,backgroundColor:"#E0E9F3", margin:"10px 10px 10px 10px", padding:"10px" }}> |
There was a problem hiding this comment.
These grid items are all the same, we should iterate over data and use a subcomponent of the Launcher, for example an OdhAppCard. Then you could clean this up like:
const odhApps = [
{
img: imgJupyter,
link: 'https://jupyterhub-odhdemo.apps.hmf.q7z3.p1.openshiftapps.com/hub/login',
description: 'Jupyter Notebooks pro ... visualisations'
},
// more data
]
<Grid hasGutter>
{odhApps.map(a => <GridItem span={3}><OdhAppCard img={a.img} link={a.link} description={a.description}/></GridItem>)}
</Grid>There was a problem hiding this comment.
Add className to components you want to style and put this css into a separate file and import it.
Always prefer external stylesheets over internal and inline. If you can't do that, you have to have a very good reason. Inline styles are very hard to override or maintain.
There was a problem hiding this comment.
Very true that external styling should be given more priority than inline style sheet.
frontend/src/common/commonStyle.css
Outdated
| @@ -0,0 +1,43 @@ | |||
| .gridItem{ | |||
There was a problem hiding this comment.
There's no reason to make a separate file and include it multiple times throughout the application. Just put this in the index.scss and it will be available throughout. If you need to override you can override them in the component and with specificity.
There was a problem hiding this comment.
Yes, I didn't think at first that putting in the index.css will available throughout. It works! Perfect!
frontend/src/Launcher/Launcher.js
Outdated
| return ( | ||
| <PageSection variant={PageSectionVariants.light}> | ||
| <Grid hasGutter className="gridItem"> | ||
| {odhApps.map(a => <GridItem span={12} sm={12} md={6} lg={4} xl={3}><OdhAppCard img={a.img} link={a.link} description={a.description} buttonName={a.buttonName} altName={a.altName}/></GridItem>)} |
There was a problem hiding this comment.
add key prop here. It's giving me a warning.
There was a problem hiding this comment.
Yes, right - key not there will give warning in the web console. It will be created.
feat: Update Argo route to argo-server
…ndatahub-io#4) * use project annotation for ds project nb routing * chore: passes host instead of the entire route and removes hub-url annotation as it is not needed * remove reference to hub-host --------- Co-authored-by: bartoszmajsak <bartosz.majsak@gmail.com>
…ndatahub-io#4) * use project annotation for ds project nb routing * chore: passes host instead of the entire route and removes hub-url annotation as it is not needed * remove reference to hub-host --------- Co-authored-by: bartoszmajsak <bartosz.majsak@gmail.com>
Please provide your approval
Thanks!