Skip to content

Conversation

@AdityaPatil22
Copy link
Collaborator

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Apr 17, 2025

⚠️ No Changeset found

Latest commit: 7968781

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

A quick overview of the scss - the use of hardcoded hex values is going to mean that this theme will not work in dark theme.

Adding the class .pf-v6-theme-dark class to the html element will flip the theme and in the demo app, you can see the side effect of using the hex values to set colors. If you can use semantic v6 tokens as hex values instead, that'll make it dark theme compatible out of the box.

otherwise, you'll need to define hex color values for these v6 semantic tokens/css variables when the dark theme class is present.

@AdityaPatil22 AdityaPatil22 self-assigned this Apr 28, 2025
Aditya Patil and others added 2 commits May 6, 2025 13:40
@ArathyKumar
Copy link
Collaborator

Hey @nicolethoen

Thank you for reviewing the PR.

@AdityaPatil22 has replaced the hardcoded hex values with CSS variables and updated the corresponding values in the dark theme as well.
Could you please take another look when you have a moment?

You can preview the changes here: https://deploy-preview-6--demo-rhds-theme-for-pf6.netlify.app/
Here is the PR for the demo project: RedHat-UX/demo-red-hat-theme-for-patternfly6#6

Copy link

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

I am happy to discuss either of my questions more. Ultimately it's up to you - The more the questions are resolved, the more future proof you are, as PF continues rolling out updates or as the rest of your ecosystem might update to use PF or PF6.

.pf-v6-c-modal-box {
--pf-v6-c-modal-box--BorderRadius: var(--rh-border-radius-sharp, 0px);
:root {
--rh-color-light-gray: #fafafa;

Choose a reason for hiding this comment

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

The color palette shipping with PF6 is the complete brand color palette. So I’m wondering if - even if they don’t 100% match the old colors, if it’s worth making sure all colors selected with this red hat brand theme are from PF6's shipped color palette? What do you think?


.pf-v6-c-table {
/* Header styles */
--pf-v6-c-table__thead--cell--FontWeight: var(--pf-v5-c-table__thead--cell--FontWeight, 700);

Choose a reason for hiding this comment

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

continuing to reference --pf-v5- css variables means you’ll need to continue shipping all of the PF5 css in addition to the PF6 CSS to get this theme to work.

Is that unavoidable & will you always have the PF5 CSS in your environment? If the rest of the environment continues forward with modernization and migrations, you’ll have to rewrite all this CSS if you ever want to stop shipping PF5 styles.

Copy link
Contributor

@markcaron markcaron left a comment

Choose a reason for hiding this comment

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

Few tokens swaps...

}

.pf-v6-c-table__expandable-row .pf-v6-c-table__expandable-row-content {
padding-inline: 0px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need px on zero.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the changes


/* Selectors for compound expandable table variants */
.pf-v6-c-table__compound-expansion-toggle.pf-m-expanded {
border-left-width: var(--rh-length-4xs, 1px);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use border tokens for borders for all these, like:

--rh-border-width-sm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the changes

.pf-v6-c-table {
/* Header styles */
--pf-v6-c-table__thead--cell--FontWeight: var(--rh-font-weight-heading-bold, 700);
--pf-v6-c-table__thead--cell--FontSize: var(--rh-font-size-code-sm, 14px);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should either be the heading font-size token or the regular text font-size token, but not the code font-size token.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the changes

/* Body styles */
--pf-v6-c-table__tbody--cell--PaddingBlockStart: var(--rh-space-xl, 24px);
--pf-v6-c-table__tbody--cell--PaddingBlockEnd: var(--rh-space-xl, 24px);
--pf-v6-c-table__tbody--cell--FontSize: var(--rh-font-size-code-md, 16px);
Copy link
Contributor

Choose a reason for hiding this comment

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

Regular text font-size token.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the changes

.pf-v6-c-table__compound-expansion-toggle:hover {
// border-left: var(--rh-length-4xs, 1px) solid var(--rh-color-gray-300, #d2d2d2);
// border-right: var(--rh-length-4xs, 1px) solid var(--rh-color-gray-300, #d2d2d2);
border-left-width: var(--rh-length-4xs, 1px);
Copy link
Contributor

Choose a reason for hiding this comment

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

Border tokens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the changes

.pf-v6-c-table.pf-m-sticky-header>.pf-v6-c-table__thead,
.pf-v6-c-table .pf-v6-c-table__thead.pf-m-nested-column-header {
// border-bottom: var(--rh-border-width-sm, 1px) solid var(--rh-color-canvas-white, #ffffff);
border-bottom-width: var(--rh-length-4xs, 1px);
Copy link
Contributor

Choose a reason for hiding this comment

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

Border token

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the changes


/* Selectors for clickable table variants */
.pf-v6-c-table tr:where(.pf-v6-c-table__tr).pf-m-clickable:is(:hover, :focus) {
box-shadow: 0 -0.1875rem 0.25rem -0.125rem rgba(3, 3, 3, 0.08), 0 0.1875rem 0.25rem -0.125rem rgba(3, 3, 3, 0.08);
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a box-shadow token, which may apply here.

--rh-box-shadow-sm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the changes

Aditya Patil and others added 2 commits June 9, 2025 16:19
@AdityaPatil22
Copy link
Collaborator Author

Hi @markcaron,
I have completed the requested changes.
Could you please re-review the PR
Thank you!

Copy link
Contributor

@markcaron markcaron left a comment

Choose a reason for hiding this comment

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

I'm not sure why it's happening, but there are some visual discrepanices between the demo deploy preview (DP) and the examples on PF6 tables:

Striped Expandable

DP is missing some padding on the left of the expandable content and it's not 100% width. This leads me to believe some of the padding overrides are off.

Screenshot 2025-06-20 at 9 16 40 AM

PF Demo (I added pf-m-striped so we can see the expandable rows better)

image

Nested sticky header

Seems to be an errant \ between the cells:
image

Compound Expandable

This might just be the HTML in the DP not matching the PF demos, but the "tabs" aren't functional, and don't open/close when clicked twice, so it's hard to tell if things match exactly. I realize this is beyond the CSS changes in this PR.

image

--pf-v6-c-button--BorderRadius: var(--rh-border-radius-default, 3px);
//--pf-v6-c-button--m-control--BorderRadius: var(--rh-border-radius-sharp, 0px);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this will be handled in a separate PR, but since we're in this button-override.scss file...

The tertiary button should have border-color: var(--rh-color-border-strong, #151515); for Red Hat brand theming.

Since we're using it in the demo deploy preview, I just noticed it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @markcaron,

  • We investigated and did some research for the Striped Expandable table variant and found that the missing padding and the 100% width issue originates from the PF-6 original implementation.
Screenshot 2025-07-01 at 5 57 39 PM
  • We have also removed the errant backslashes "" between the cells.

  • Additionally, we reviewed the Compound Expandable table variant and tested it in both the local environment and the demo project. The tabs are functioning as expected and are consistent with the original PF-6 table specifications. If possible, could you please provide us with additional steps that we can follow to reproduce the issue that you are facing.

Screen.Recording.2025-07-02.at.5.21.14.PM.mov
  • As part of the pf-button component updates, we have added border-color: var(--rh-color-border-strong, #151515); to the tertiary button styling.

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.

5 participants