-
Notifications
You must be signed in to change notification settings - Fork 22
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
add InvalidObject #45
Conversation
config/setupTests.js
Outdated
@@ -3,8 +3,13 @@ import 'whatwg-fetch'; | |||
import 'babel-polyfill'; | |||
import '@testing-library/jest-dom'; | |||
|
|||
import Adapter from '@cfaester/enzyme-adapter-react-18'; | |||
import { configure } from 'enzyme'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want enzyme for testing. It's not maintained anymore. We are using the @testing-library/react
for unit testing.
package.json
Outdated
@@ -63,8 +63,15 @@ | |||
"react": "^18", | |||
"react-dom": "^18", | |||
"rimraf": "^2.6.2", | |||
"sass": "^1.68.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sass should not be required as we use CSS in JS
<Title headingLevel="h1" size="xl" className={classes['ins-c-text__sorry']}> | ||
Let's find you a new one. Try a new search or return home. | ||
</Title> | ||
<Button variant="link" component="a" href={`${window.location.origin}${isBeta()}`}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link should be a prop with a /
fallback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this should be a prop but adding props is going to break all the existing instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@InsaneZein let's do breaking changes if necessary. When apps are migrating to our new repo, they need to expect some changes. There probably won't be a better opportunity to make the changes than now. Just we need to prepare a wrapper in FEC after this PR is merged which will use this component and make its new API compatible with the old one to ensure temporary compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! Sounds good to me. I'll make the changes. I'll probably need some help with the wrapper if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, let's clean everything that should be cleaned. We can deal with the compatibility afterwards - a similar wrapper is here https://github.com/RedHatInsights/frontend-components/pull/1764/files#diff-de3a34ca94b2dff88e7502e7f8147238c7e316270e108643e104cbee1af26cb3
that one was to add a Red Hat specific description which we don't want in PatternFly extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The components are moving to a different repository. It is expected to make breaking changes. This library is not a continuation of FEC, but a generalization of the components in FEC and detaching any relation to HCC to allow the usage outside of HCC.
flexDirection: "column", | ||
alignItems: "center", | ||
textAlign: "center", | ||
h1: {marginBottom: "30px"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be using the PF token values instead of constants where possible. Alternatively, we can use the PF classes like pf-v5-u-mb-lg
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should stick to tokens as it will be needed when theming is released.
import { createUseStyles } from 'react-jss' | ||
|
||
const useStyles = createUseStyles({ | ||
"ins-c-component_invalid-component": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The names of classes should not be specific to HCC. CSS in JS also create unique hashes for class names co we don't have to worry about having conventions that much.
@fhlavac do we have some convention for this repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's omit the "ins-c-" prefix
classNames should be in camelCase containing component's name and then follow with more details to describe its purpose, so something like
"invalidObject" for the component wrapper, "invalidObjectTitle" for a title in that component, and so on
I will enhance our contribution guide with this info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was adding pfcg instead of ins-c @fhlavac I can drop that if that's what we decide. Also should we now be using camel case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dlabaj yes please, sorry for the changes. Other components should be already using it
}) | ||
|
||
// Don't use chrome here because the 404 page on landing does not use chrome | ||
const isBeta = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be sent as a prop. Beta is not a generic PF concept. Or rather it should not exist at all and all affected attributes should be just props.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few changes
package.json
Outdated
"serve": "^14.1.2", | ||
"ts-jest": "29.0.3", | ||
"whatwg-fetch": "^3.6.2" | ||
}, | ||
"dependencies": { | ||
"@cfaester/enzyme-adapter-react-18": "^0.7.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the enzyme dependencies.
.../patternfly-docs/content/extensions/component-groups/examples/InvalidObject/InvalidObject.md
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,11 @@ | |||
import InvalidObject from './InvalidObject'; | |||
import React from 'react'; | |||
import { mount } from 'enzyme'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update to use react testing library. If you need any help feel free to reach out.
import { createUseStyles } from 'react-jss' | ||
|
||
const useStyles = createUseStyles({ | ||
"ins-c-component_invalid-component": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was adding pfcg instead of ins-c @fhlavac I can drop that if that's what we decide. Also should we now be using camel case?
flexDirection: "column", | ||
alignItems: "center", | ||
textAlign: "center", | ||
h1: {marginBottom: "30px"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should stick to tokens as it will be needed when theming is released.
@InsaneZein Can you run the lint command with --fix to fix the linting errors? |
.../patternfly-docs/content/extensions/component-groups/examples/InvalidObject/InvalidObject.md
Outdated
Show resolved
Hide resolved
.../patternfly-docs/content/extensions/component-groups/examples/InvalidObject/InvalidObject.md
Outdated
Show resolved
Hide resolved
.../patternfly-docs/content/extensions/component-groups/examples/InvalidObject/InvalidObject.md
Outdated
Show resolved
Hide resolved
packages/module/src/InvalidObject/__snapshots__/InvalidObject.test.js.snap
Outdated
Show resolved
Hide resolved
Co-authored-by: Donald Labaj <dlabaj@redhat.com>
…roups/examples/InvalidObject/InvalidObject.md Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com>
…roups/examples/InvalidObject/InvalidObject.md Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com>
|
||
export interface InvalidObjectProps { | ||
/** Indicates if an additional "/beta" string should be added to when navigating back to the home screen. */ | ||
isBeta?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edonehoo This is the only prop that we have for this component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
export interface InvalidObjectProps { | ||
/** Indicates if an additional "/beta" string should be added to when navigating back to the home screen. */ | ||
isBeta?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think rather than the isBeta
prop we should allow customizing the props it affects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the whole href should be a prop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@InsaneZein I would vote for being consistent with the NotAuthorized
component
<Button variant="primary" component="a" href={toLandingPageUrl}> |
…roups/examples/InvalidObject/InvalidObject.md Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com>
|
||
import Icon404 from './icon-404'; | ||
import React from 'react'; | ||
import classNames from 'classnames'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, let's use clsx with a slightly smaller bundle size instead of classnames, current main already has that change - it's just renaming the function.
import { createUseStyles } from 'react-jss'; | ||
|
||
const useStyles = createUseStyles({ | ||
Icon404: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Icon404: { | |
icon404: { |
'cls-1': { fill: '#fff', fillRule: 'evenodd' }, | ||
'cls-3': { fillRule: 'evenodd' }, | ||
'cls-2': { opacity: 0.5 }, | ||
'cls-4': { mask: 'url(#mask)' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should be in camelCase. Also we could rename the classes to be more descriptive - what part of the icon they affect
const Icon404: React.FunctionComponent = () => { | ||
const classes = useStyles(); | ||
return ( | ||
// eslint-disable-next-line max-len |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's rename this file to match contribution rules - maybe something likeNotFoundIcon
could work and move it to a separate directory /src/NotFoundIcon. We should not have multiple components under the same directory (you don't have to create a separate example for it, it's just an icon - but we could show the icon component as a part of the InvalidObject example a similar way as DetailsPage subcomponents are presented)
import { createUseStyles } from 'react-jss' | ||
|
||
const useStyles = createUseStyles({ | ||
"invalidObject": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"invalidObject": { | |
invalidObject: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the styling is needed with the EmptyState, it works even without it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I got rid of it. It does work without it.
…nent-groups into add-invalidobject
packages/module/package.json
Outdated
@@ -48,6 +48,7 @@ | |||
"@types/react": "^18.0.0", | |||
"@types/react-dom": "^18.0.0", | |||
"@types/react-router-dom": "^5.3.3", | |||
"classnames": "^2.2.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use clsx instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops forgot to take this out of package. fixed.
.../patternfly-docs/content/extensions/component-groups/examples/InvalidObject/InvalidObject.md
Outdated
Show resolved
Hide resolved
.../patternfly-docs/content/extensions/component-groups/examples/InvalidObject/InvalidObject.md
Outdated
Show resolved
Hide resolved
...fly-docs/content/extensions/component-groups/examples/InvalidObject/InvalidObjectExample.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to create a separate index file under the NotFoundIcon directory to export this component than export it under InvalidObject
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Just a few little comments, we are close! |
…roups/examples/InvalidObject/InvalidObject.md Co-authored-by: Filip Hlavac <50696716+fhlavac@users.noreply.github.com>
Co-authored-by: Filip Hlavac <50696716+fhlavac@users.noreply.github.com>
…roups/examples/InvalidObject/InvalidObjectExample.tsx Co-authored-by: Filip Hlavac <50696716+fhlavac@users.noreply.github.com>
…roups/examples/InvalidObject/InvalidObject.md Co-authored-by: Filip Hlavac <50696716+fhlavac@users.noreply.github.com>
@fhlavac I think I've addressed everything. |
All comments seem to be adressed
🎉 This PR is included in version 1.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 5.0.0-prerelease.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
adds InvalidObject with example and test