add InvalidObject by InsaneZein · Pull Request #45 · patternfly/react-component-groups

InsaneZein · 2023-10-03T21:05:18Z

adds InvalidObject with example and test

Hyperkid123 · 2023-10-04T07:11:52Z

config/setupTests.js

 import '@testing-library/jest-dom';

+import Adapter from '@cfaester/enzyme-adapter-react-18';
+import { configure } from 'enzyme';


We don't want enzyme for testing. It's not maintained anymore. We are using the @testing-library/react for unit testing.

Hyperkid123 · 2023-10-04T07:12:30Z

package.json

    "react": "^18",
    "react-dom": "^18",
    "rimraf": "^2.6.2",
+    "sass": "^1.68.0",


sass should not be required as we use CSS in JS

Hyperkid123 · 2023-10-04T07:13:43Z

packages/module/src/InvalidObject/InvalidObject.tsx

+      <Title headingLevel="h1" size="xl" className={classes['ins-c-text__sorry']}>
+        Let&apos;s find you a new one. Try a new search or return home.
+      </Title>
+      <Button variant="link" component="a" href={`${window.location.origin}${isBeta()}`}>


The link should be a prop with a / fallback.

I agree this should be a prop but adding props is going to break all the existing instances.

@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.

Ok! Sounds good to me. I'll make the changes. I'll probably need some help with the wrapper if possible.

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

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.

Hyperkid123 · 2023-10-04T07:15:15Z

packages/module/src/InvalidObject/InvalidObject.tsx

+    flexDirection: "column",
+    alignItems: "center",
+    textAlign: "center",
+    h1: {marginBottom: "30px"},


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.

We should stick to tokens as it will be needed when theming is released.

Hyperkid123 · 2023-10-04T07:16:24Z

packages/module/src/InvalidObject/InvalidObject.tsx

+import { createUseStyles } from 'react-jss'
+
+const useStyles = createUseStyles({
+  "ins-c-component_invalid-component": {


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?

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

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?

@dlabaj yes please, sorry for the changes. Other components should be already using it

Hyperkid123 · 2023-10-04T07:16:56Z

packages/module/src/InvalidObject/InvalidObject.tsx

+})
+
+// Don't use chrome here because the 404 page on landing does not use chrome
+const isBeta = () => {


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.

dlabaj

A few changes

dlabaj · 2023-10-05T13:45:43Z

package.json

    "whatwg-fetch": "^3.6.2"
+  },
+  "dependencies": {
+    "@cfaester/enzyme-adapter-react-18": "^0.7.1",


Can you remove the enzyme dependencies.

.../patternfly-docs/content/extensions/component-groups/examples/InvalidObject/InvalidObject.md

dlabaj · 2023-10-05T13:47:25Z

packages/module/src/InvalidObject/InvalidObject.test.js

@@ -0,0 +1,11 @@
+import InvalidObject from './InvalidObject';
+import React from 'react';
+import { mount } from 'enzyme';


Update to use react testing library. If you need any help feel free to reach out.

dlabaj · 2023-10-05T13:48:38Z

packages/module/src/InvalidObject/InvalidObject.tsx

+import { createUseStyles } from 'react-jss'
+
+const useStyles = createUseStyles({
+  "ins-c-component_invalid-component": {


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?

dlabaj · 2023-10-05T13:49:18Z

packages/module/src/InvalidObject/InvalidObject.tsx

+    flexDirection: "column",
+    alignItems: "center",
+    textAlign: "center",
+    h1: {marginBottom: "30px"},


We should stick to tokens as it will be needed when theming is released.

dlabaj · 2023-10-05T13:52:05Z

@InsaneZein Can you run the lint command with --fix to fix the linting errors?

…nent-groups into add-invalidobject

.../patternfly-docs/content/extensions/component-groups/examples/InvalidObject/InvalidObject.md

packages/module/src/InvalidObject/InvalidObject.tsx

packages/module/src/InvalidObject/__snapshots__/InvalidObject.test.js.snap

Co-authored-by: Donald Labaj <dlabaj@redhat.com>

…roups/examples/InvalidObject/InvalidObject.md Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com>

dlabaj · 2023-10-05T17:00:36Z

packages/module/src/InvalidObject/InvalidObject.tsx

+
+export interface InvalidObjectProps {
+  /** Indicates if an additional "/beta" string should be added to when navigating back to the home screen. */
+  isBeta?: boolean


@edonehoo This is the only prop that we have for this component.

dlabaj

LGTM

Hyperkid123 · 2023-10-09T07:11:14Z

packages/module/src/InvalidObject/InvalidObject.tsx

+
+export interface InvalidObjectProps {
+  /** Indicates if an additional "/beta" string should be added to when navigating back to the home screen. */
+  isBeta?: boolean


I still think rather than the isBeta prop we should allow customizing the props it affects.

so the whole href should be a prop?

@InsaneZein I would vote for being consistent with the NotAuthorized component

react-component-groups/packages/module/src/NotAuthorized/NotAuthorized.tsx

Line 53 in 6f42868

<Button variant="primary" component="a" href={toLandingPageUrl}>

…roups/examples/InvalidObject/InvalidObject.md Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com>

fhlavac · 2023-10-10T08:51:03Z

packages/module/src/InvalidObject/InvalidObject.tsx

+
+import Icon404 from './icon-404';
+import React from 'react';
+import classNames from 'classnames';


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.

fhlavac · 2023-10-10T08:52:21Z

packages/module/src/InvalidObject/icon-404.tsx

+import { createUseStyles } from 'react-jss';
+
+const useStyles = createUseStyles({
+  Icon404: {


Suggested change

Icon404: {

icon404: {

fhlavac · 2023-10-10T08:52:42Z

packages/module/src/InvalidObject/icon-404.tsx

+  'cls-1': { fill: '#fff', fillRule: 'evenodd' },
+  'cls-3': { fillRule: 'evenodd' },
+  'cls-2': { opacity: 0.5 },
+  'cls-4': { mask: 'url(#mask)' }


these should be in camelCase. Also we could rename the classes to be more descriptive - what part of the icon they affect

fhlavac · 2023-10-10T08:54:39Z

packages/module/src/InvalidObject/icon-404.tsx

+const Icon404: React.FunctionComponent = () => {
+  const classes = useStyles();
+  return (
+    // eslint-disable-next-line max-len


this should not be needed

fhlavac · 2023-10-10T08:58:35Z

packages/module/src/InvalidObject/icon-404.tsx

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)

fhlavac · 2023-10-10T08:59:04Z

packages/module/src/InvalidObject/InvalidObject.tsx

+import { createUseStyles } from 'react-jss'
+
+const useStyles = createUseStyles({
+  "invalidObject": {


Suggested change

"invalidObject": {

invalidObject: {

I don't think the styling is needed with the EmptyState, it works even without it

Yeah I got rid of it. It does work without it.

…nent-groups into add-invalidobject

fhlavac · 2023-10-19T12:38:39Z

packages/module/package.json

    "@types/react": "^18.0.0",
    "@types/react-dom": "^18.0.0",
    "@types/react-router-dom": "^5.3.3",
+    "classnames": "^2.2.5",


we should use clsx instead

oops forgot to take this out of package. fixed.

.../patternfly-docs/content/extensions/component-groups/examples/InvalidObject/InvalidObject.md

...fly-docs/content/extensions/component-groups/examples/InvalidObject/InvalidObjectExample.tsx

packages/module/src/InvalidObject/index.ts

fhlavac · 2023-10-19T12:55:51Z

packages/module/src/NotFoundIcon/NotFoundIcon.tsx

It would be better to create a separate index file under the NotFoundIcon directory to export this component than export it under InvalidObject

fhlavac · 2023-10-19T13:02:03Z

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>

InsaneZein · 2023-10-19T14:25:51Z

Just a few little comments, we are close!

@fhlavac I think I've addressed everything.

packages/module/src/InvalidObject/InvalidObject.tsx

All comments seem to be adressed

github-actions · 2023-10-24T13:09:35Z

🎉 This PR is included in version 1.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions · 2023-11-01T08:43:19Z

🎉 This PR is included in version 5.0.0-prerelease.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

InsaneZein added 2 commits October 3, 2023 16:01

add InvalidObject

698ae1f

Merge branch 'main' into add-invalidobject

7b66abc

Hyperkid123 previously requested changes Oct 4, 2023

View reviewed changes

dlabaj self-requested a review October 5, 2023 13:44

dlabaj requested changes Oct 5, 2023

View reviewed changes

dlabaj requested a review from edonehoo October 5, 2023 14:07

InsaneZein added 3 commits October 5, 2023 09:08

remove enzyme and sass; update styling class names; add isbeta prop

a9068b8

Merge branch 'add-invalidobject' of github.com:InsaneZein/react-compo…

b1561d4

…nent-groups into add-invalidobject

remove enzyme dependencies

e8f9cf9

edonehoo reviewed Oct 5, 2023

View reviewed changes

dlabaj linked an issue Oct 5, 2023 that may be closed by this pull request

Migrate InvalidObject to component-groups #47

Closed

InsaneZein added 2 commits October 5, 2023 09:41

change test from js to tsx

51e9b36

update snap

45827e8

dlabaj reviewed Oct 5, 2023

View reviewed changes

packages/module/src/InvalidObject/InvalidObject.tsx Outdated Show resolved Hide resolved

dlabaj reviewed Oct 5, 2023

View reviewed changes

packages/module/src/InvalidObject/__snapshots__/InvalidObject.test.js.snap Outdated Show resolved Hide resolved

InsaneZein and others added 3 commits October 5, 2023 10:14

Update packages/module/src/InvalidObject/InvalidObject.tsx

c6278ef

Co-authored-by: Donald Labaj <dlabaj@redhat.com>

Update packages/module/patternfly-docs/content/extensions/component-g…

c313408

…roups/examples/InvalidObject/InvalidObject.md Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com>

Update packages/module/patternfly-docs/content/extensions/component-g…

d62af41

…roups/examples/InvalidObject/InvalidObject.md Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com>

dlabaj reviewed Oct 5, 2023

View reviewed changes

dlabaj approved these changes Oct 5, 2023

View reviewed changes

InsaneZein requested a review from Hyperkid123 October 6, 2023 12:59

Hyperkid123 reviewed Oct 9, 2023

View reviewed changes

Update packages/module/patternfly-docs/content/extensions/component-g…

be4b7a9

…roups/examples/InvalidObject/InvalidObject.md Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com>

fhlavac reviewed Oct 10, 2023

View reviewed changes

InsaneZein added 3 commits October 18, 2023 10:58

Merge branch 'add-invalidobject' of github.com:InsaneZein/react-compo…

297ad24

…nent-groups into add-invalidobject

fix merge issues

3d64665

update snapshot

26adcbf

fhlavac reviewed Oct 19, 2023

View reviewed changes

.../patternfly-docs/content/extensions/component-groups/examples/InvalidObject/InvalidObject.md Outdated Show resolved Hide resolved

fhlavac reviewed Oct 19, 2023

View reviewed changes

.../patternfly-docs/content/extensions/component-groups/examples/InvalidObject/InvalidObject.md Outdated Show resolved Hide resolved

fhlavac reviewed Oct 19, 2023

View reviewed changes

...fly-docs/content/extensions/component-groups/examples/InvalidObject/InvalidObjectExample.tsx Outdated Show resolved Hide resolved

fhlavac reviewed Oct 19, 2023

View reviewed changes

packages/module/src/InvalidObject/index.ts Outdated Show resolved Hide resolved

fhlavac reviewed Oct 19, 2023

View reviewed changes

InsaneZein and others added 8 commits October 19, 2023 08:26

add notfoundicon index

310b9fd

Update packages/module/patternfly-docs/content/extensions/component-g…

df39ff2

…roups/examples/InvalidObject/InvalidObject.md Co-authored-by: Filip Hlavac <50696716+fhlavac@users.noreply.github.com>

Update packages/module/src/InvalidObject/index.ts

63ebd31

Co-authored-by: Filip Hlavac <50696716+fhlavac@users.noreply.github.com>

Update packages/module/patternfly-docs/content/extensions/component-g…

5d53c04

…roups/examples/InvalidObject/InvalidObjectExample.tsx Co-authored-by: Filip Hlavac <50696716+fhlavac@users.noreply.github.com>

Update packages/module/patternfly-docs/content/extensions/component-g…

b4dcfc5

…roups/examples/InvalidObject/InvalidObject.md Co-authored-by: Filip Hlavac <50696716+fhlavac@users.noreply.github.com>

remove unncessary styling

0bcc92f

remove return because of lint

265f1a7

update snapshot

42d1d36

fhlavac reviewed Oct 19, 2023

View reviewed changes

packages/module/src/InvalidObject/InvalidObject.tsx Outdated Show resolved Hide resolved

Update packages/module/src/InvalidObject/InvalidObject.tsx

aabc7ab

fhlavac approved these changes Oct 19, 2023

View reviewed changes

fhlavac requested a review from Hyperkid123 October 19, 2023 14:31

fhlavac merged commit dc22fb3 into patternfly:main Oct 20, 2023

github-actions bot added the released on @prerelease label Oct 24, 2023

InsaneZein deleted the add-invalidobject branch October 24, 2023 20:42

github-actions bot mentioned this pull request Apr 21, 2025

The automated release is failing 🚨 #692

Closed

Conversation

InsaneZein commented Oct 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhlavac Oct 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlabaj commented Oct 5, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhlavac Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fhlavac Oct 5, 2023 •

edited

Loading

fhlavac Oct 10, 2023 •

edited

Loading

fhlavac Oct 19, 2023 •

edited

Loading

fhlavac commented Oct 19, 2023 •

edited

Loading