add OpenShift LogSnippet #105

InsaneZein · 2024-02-08T16:23:20Z

closes #99

nicolethoen · 2024-02-08T16:27:48Z

packages/module/src/LogSnippet/LogSnippet.tsx

+    borderLeft: '2px solid var(--pf-v5-global--danger-color--100)',
+    display: 'flex',
+    flexDirection: 'column',
+    marginLeft: '6px',


Ther are PF CSS variables for margins/paddings we should be using

nicolethoen · 2024-02-08T16:28:32Z

packages/module/src/LogSnippet/LogSnippet.tsx

+const useStyles = createUseStyles({
+  logSnippet: {
+    borderLeft: '2px solid var(--pf-v5-global--danger-color--100)',
+    display: 'flex',


There is a PF layout we should be able to use rather than hardcoding layouts into the CSS. https://www.patternfly.org/layouts/flex

dlabaj · 2024-02-08T16:26:10Z

packages/module/src/LogSnippet/LogSnippet.tsx

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


We usually use clsx with this. Is there a reason we aren't here? I think we should use a layout instead of the div and p below.

I thought clsx was just for conditional rendering

dlabaj · 2024-02-08T16:26:29Z

packages/module/src/LogSnippet/LogSnippet.tsx

+  },
+});
+
+interface LogSnippetProps {


This should be exported.

dlabaj · 2024-02-08T16:31:38Z

packages/module/src/LogSnippet/LogSnippet.tsx

+  const classes = useStyles();
+
+  return (
+    <div className={classes.logSnippet}>


Is there a PF layout you can use instead of these div and paragraphs?

I've replaced the div with the flex layout. I'm not sure what to do about the paragraph. Is there something you have in mind?

@InsaneZein we could use https://www.patternfly.org/components/text

dlabaj · 2024-02-08T16:32:50Z

@InsaneZein Looks like linting is failing.

fhlavac · 2024-02-12T10:21:56Z

...module/patternfly-docs/content/extensions/component-groups/examples/LogSnippet/LogSnippet.md

+subsection: Component groups
+# Sidenav secondary level section
+# should be the same for all markdown files
+id: LogSnippet


Suggested change

id: LogSnippet

id: Log snippet

fhlavac · 2024-02-12T10:24:29Z

...atternfly-docs/content/extensions/component-groups/examples/LogSnippet/LogSnippetExample.tsx

+  url: https://raw.githubusercontent.com/Azure-Samples/helm-charts/master/docs`;
+
+  return <>
+    <LogSnippet message='A log snippet' logSnippet={code} />


We could maybe adjust the label to make it more explicit what is its purpose

Suggested change

<LogSnippet message='A log snippet' logSnippet={code} />

<LogSnippet message='Failure - check logs for details' logSnippet={code} />

fhlavac · 2024-02-12T10:27:19Z

packages/module/src/LogSnippet/LogSnippet.tsx

+
+const useStyles = createUseStyles({
+  logSnippet: {
+    borderLeft: '2px solid var(--pf-v5-global--danger-color--100)',


The red border should be probably optional or at least overridable with custom class. We may also need a different variant than danger

@nicolethoen please, what do you think about the border vs PF habits? Do we want to have it visible by default with an option to hide and select color variant? Or maybe do not show it and if OpenShift wants it, they can pass a custom className? Thank you

There's a few ways it could be done.
An additional prop could be used - either a flag like isBorderLeft, or an enum like borderLeftVariant='red|<other options>.
we could even expose a customBorderLeftColor which could override this style if we want red to be the default?

I would probably vote for borderLeftVariant with colors and "hidden" option. Thoughts? @InsaneZein @nicolethoen

Yeah I'm good with that. I'll get to it.

I talked with @fhlavac we're thinking of just passing in "danger", "success", "info", "warning", or "hidden" so the variants would be --pf-v5-global--danger-color--100 --pf-v5-global--success-color--100 --pf-v5-global--info-color--100 --pf-v5-global--warning-color--100

fhlavac · 2024-02-12T10:28:22Z

packages/module/src/LogSnippet/LogSnippet.tsx

+export interface LogSnippetProps {
+  /** log snippet or code you would like to display */
+  logSnippet?: string;
+  /** message to appear above the log snippet */


Suggested change

/** message to appear above the log snippet */

/** Message to appear above the log snippet */

fhlavac · 2024-02-12T10:28:51Z

packages/module/src/LogSnippet/LogSnippet.tsx

+});
+
+export interface LogSnippetProps {
+  /** log snippet or code you would like to display */


Suggested change

/** log snippet or code you would like to display */

/** Log snippet or code to be displayed */

fhlavac · 2024-02-12T10:29:19Z

packages/module/src/LogSnippet/LogSnippet.tsx

+    borderLeft: '2px solid var(--pf-v5-global--danger-color--100)',
+    marginLeft: 'var(--pf-v5-global--spacer--sm)',
+    padding: 'var(--pf-v5-global--spacer--sm) 0 var(--pf-v5-global--spacer--sm) var(--pf-v5-global--spacer--sm)',
+    backgroundColor: 'white'


we may want to use PF variable as well

fhlavac · 2024-02-12T10:30:14Z

packages/module/src/LogSnippet/LogSnippet.tsx

+    backgroundColor: 'white'
+  },
+  statusMessage: {
+    marginBottom: '10px',


could be replaced by PF variable depending on UX suggestions

fhlavac · 2024-02-12T10:50:42Z

@InsaneZein looks good, just some styling/cleanup notes. Thank you 🎉

...module/patternfly-docs/content/extensions/component-groups/examples/LogSnippet/LogSnippet.md

fhlavac · 2024-02-14T07:44:15Z

packages/module/src/LogSnippet/LogSnippet.tsx

+  /** Log snippet or code to be displayed */
+  logSnippet?: string;
+  /** Message to appear above the log snippet */
+  message: string;


Thinking if it wouldn't be better to make the message a ReactNode to be easier customizable. If string is passed, we can wrap it inside Text as we do now

fhlavac · 2024-02-14T07:45:49Z

packages/module/src/LogSnippet/LogSnippet.tsx

+  const classes = useStyles();
+
+  return (
+    <Flex direction={{ default: 'column' }} className={classes.logSnippet}>


@InsaneZein what do you think about extending FlexProps in the LogSnippetProps to make this more customizable?

fhlavac · 2024-02-15T16:38:48Z

packages/module/src/LogSnippet/LogSnippet.tsx

+
+const useStyles = createUseStyles({
+  logSnippet: {
+    borderLeft: '2px solid var(--pf-v5-global--danger-color--100)',


also, we could use the PF variable for the border width - --pf-v5-global--BorderWidth--md

…nto add-logsnippet

patternfly-build · 2024-02-20T18:08:00Z

Preview: https://react-component-groups-pr-component-groups-105.surge.sh

A11y report: https://react-component-groups-pr-component-groups-105-a11y.surge.sh

…t-groups into add-logsnippet

dlabaj · 2024-02-20T19:38:03Z

@InsaneZein If you rebase this with main it should now pass.

dlabaj

LGTM

…nto add-logsnippet

nicolethoen · 2024-02-20T20:24:27Z

packages/module/src/LogSnippet/LogSnippet.tsx

+  logSnippet: {
+    marginLeft: 'var(--pf-v5-global--spacer--sm)',
+    padding: 'var(--pf-v5-global--spacer--sm) 0 var(--pf-v5-global--spacer--sm) var(--pf-v5-global--spacer--sm)',
+    backgroundColor: 'var(--pf-v5-global--palette--black-100)'


I'd advocate that we use a more meaningful variable here: maybe: --pf-v5-global--BackgroundColor--200

jessiehuff

LGTM!

fhlavac · 2024-02-22T08:14:08Z

@InsaneZein one last thing - we seem to be missing feat(xx): prefix in at least one commit message to trigger the release. Can you please add it?

github-actions · 2024-02-22T15:37:32Z

🎉 This PR is included in version 5.1.0-prerelease.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

add OpenShift LogSnippet

83d46d5

InsaneZein requested review from nicolethoen and fhlavac February 8, 2024 16:23

nicolethoen requested changes Feb 8, 2024

View reviewed changes

dlabaj requested changes Feb 8, 2024

View reviewed changes

InsaneZein added 2 commits February 9, 2024 17:07

fix layout; switch to pf variables

3af9de9

add test

e63ddfa

fhlavac reviewed Feb 12, 2024

View reviewed changes

fhlavac reviewed Feb 13, 2024

View reviewed changes

...module/patternfly-docs/content/extensions/component-groups/examples/LogSnippet/LogSnippet.md Show resolved Hide resolved

InsaneZein added 2 commits February 13, 2024 10:22

pf variables and fix typos

3fcba6e

fix snapshot

6ce785e

fhlavac reviewed Feb 14, 2024

View reviewed changes

fhlavac reviewed Feb 15, 2024

View reviewed changes

InsaneZein added 8 commits February 15, 2024 15:17

wip: add leftBorderVariant

e510823

fix dynamic styling

e659560

update snapshot

bae7d6c

remove old redborder class

9959299

update snapshot again

b8327d4

add types to createuseStyles

3d000d5

revert leftborder default to danger

8b7385c

Merge branch 'main' of github.com:patternfly/react-component-groups i…

fb101c3

…nto add-logsnippet

InsaneZein added 10 commits February 20, 2024 13:04

pf variables and fix typos

7689bd2

fix snapshot

558893c

wip: add leftBorderVariant

49a1255

fix dynamic styling

5bbc8f4

update snapshot

1a34f9b

remove old redborder class

a2b1258

update snapshot again

f6a1506

add types to createuseStyles

bc7cc0f

revert leftborder default to danger

b21ccdf

clean up code

9c8dbf8

dlabaj force-pushed the add-logsnippet branch from 96c265e to 9c8dbf8 Compare February 20, 2024 18:05

InsaneZein added 2 commits February 20, 2024 12:56

Merge branch 'add-logsnippet' of github.com:patternfly/react-componen…

588d44f

…t-groups into add-logsnippet

more cleanup

8f2074a

dlabaj approved these changes Feb 20, 2024

View reviewed changes

Merge branch 'main' of github.com:patternfly/react-component-groups i…

0d4e71c

…nto add-logsnippet

nicolethoen reviewed Feb 20, 2024

View reviewed changes

fhlavac approved these changes Feb 21, 2024

View reviewed changes

jessiehuff approved these changes Feb 21, 2024

View reviewed changes

InsaneZein requested a review from nicolethoen February 21, 2024 21:27

feat(logsnippet): add log snippet

ea38872

InsaneZein force-pushed the add-logsnippet branch from 981449f to ea38872 Compare February 22, 2024 13:51

nicolethoen approved these changes Feb 22, 2024

View reviewed changes

nicolethoen merged commit e230962 into main Feb 22, 2024

github-actions bot added the released on @prerelease label Feb 22, 2024

fhlavac deleted the add-logsnippet branch February 22, 2024 16:42

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

The automated release is failing 🚨 #692

Closed

	<LogSnippet message='A log snippet' logSnippet={code} />
	<LogSnippet message='Failure - check logs for details' logSnippet={code} />

	/** message to appear above the log snippet */
	/** Message to appear above the log snippet */

	/** log snippet or code you would like to display */
	/** Log snippet or code to be displayed */

add OpenShift LogSnippet #105

add OpenShift LogSnippet #105

Uh oh!

Conversation

InsaneZein commented Feb 8, 2024 • edited by fhlavac Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

dlabaj commented Feb 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhlavac Feb 12, 2024 • 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!

fhlavac Feb 14, 2024 • 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

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 commented Feb 12, 2024

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!

patternfly-build commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlabaj commented Feb 20, 2024

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!

jessiehuff left a comment

Choose a reason for hiding this comment

Uh oh!

fhlavac commented Feb 22, 2024

InsaneZein commented Feb 8, 2024 •

edited by fhlavac

Loading

fhlavac Feb 12, 2024 •

edited

Loading

fhlavac Feb 14, 2024 •

edited

Loading

patternfly-build commented Feb 20, 2024 •

edited

Loading