-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(v6): Update to PatternFly V6 Versions #54
Conversation
packages/module/patternfly-docs/content/examples/catalogTile.css
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/examples/catalogTile.css
Outdated
Show resolved
Hide resolved
packages/module/sass/react-catalog-view-extension/_catalog-tile.scss
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/examples/catalogTile.css
Outdated
Show resolved
Hide resolved
Here is a link to the Figma mocks. |
packages/module/patternfly-docs/content/examples/FilterSidePanel.md
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.
LGTM @andrew-ronaldson any comments on this pr?
@jessiehuff Can you add a surge site to this PR? |
Surge link for preview: https://catalog-view-v6.surge.sh/ |
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 looks like the catalog view heading font is rendering oddly (below)... I think it's just the font-weight/size. I can make a suggestion if it's something we want to fix.
The top border on featured tile looks a little odd with the rounded corners. I think that's a question for the design folks on how to best handle that.
Also I'm sure there are lots of things that could still use updating - how detailed do we want to be in this PR? There are hex codes for colors in the examples, font-sizes/weights, margins/paddings, border-widths, etc that could all be converted to tokens to be closer to the new PF6 styles.
@@ -33,7 +33,7 @@ import GlobeIcon from '@patternfly/react-icons/dist/esm/icons/globe-icon'; | |||
label="Certified Level" | |||
value={ | |||
<span> | |||
<OkIcon style={{color: 'var(--pf-v5-global--success-color--100)'}} /> Certified | |||
<OkIcon style={{color: 'var(--pf-v6-global--success-color--100)'}} /> Certified |
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.
If this is just the default success color, it's --pf-t--global--color--status--success--default
. If you want a reference for icon status colors, you can look at the icon component styles and look for --m-[status]
, like --m-success
and see which token is used
<OkIcon style={{color: 'var(--pf-v6-global--success-color--100)'}} /> Certified | |
<OkIcon style={{color: 'var(--pf-t--global--color--status--success--default)'}} /> Certified |
@@ -20,6 +20,6 @@ | |||
} | |||
|
|||
.catalog-item-header-pf-subtitle { | |||
color: var(--pf-v5-global--Color--200); | |||
color: var(--pf-v6-global--Color--200); |
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 new secondary text color token is --pf-t--global--text--color--subtle
color: var(--pf-v6-global--Color--200); | |
color: var(--pf-t--global--text--color--subtle); |
@@ -27,7 +27,7 @@ | |||
} | |||
|
|||
.catalog-tile-pf-subtitle { | |||
color: var(--pf-v5-global--Color--200); | |||
color: var(--pf-t--global--text--color--subtle); |
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.
👍
$vertical-tab-pf-color: var(--pf-v6-global--Color--100) !default; | ||
$vertical-tab-pf-active-color: var(--pf-v6-global--active-color--100) !default; |
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'm not sure where all these vars are used, but looks like this is probably the update we want to make? FWIW they're used to create the vars below in the :root
block - --vertical-tab-pf-color
and --vertical-tab-pf-active-color
$vertical-tab-pf-color: var(--pf-v6-global--Color--100) !default; | |
$vertical-tab-pf-active-color: var(--pf-v6-global--active-color--100) !default; | |
$vertical-tab-pf-color: var(--pf-t--global--text--color--regular) !default; | |
$vertical-tab-pf-active-color: var(--pf-t--global--text--color--brand--clicked) !default; |
It looks like you updated the use of --vertical-tab-pf-active-color
in these 3 spots to just use the brand tokens directly:
react-catalog-view/packages/module/sass/react-catalog-view-extension/_vertical-tabs.scss
Line 26 in bafc9de
color: var(--pf-t--global--text--color--brand--hover); react-catalog-view/packages/module/sass/react-catalog-view-extension/_vertical-tabs.scss
Line 44 in bafc9de
color: var(--pf-t--global--text--color--brand--clicked); react-catalog-view/packages/module/sass/react-catalog-view-extension/_vertical-tabs.scss
Line 47 in bafc9de
background: var( --pf-t--global--border--color--brand--clicked);
IMO that's fine, but you may also consider updating this line to use --pf-t--global--text--color--regular
, which would be inline with the 3 updates above
react-catalog-view/packages/module/sass/react-catalog-view-extension/_vertical-tabs.scss
Line 16 in bafc9de
color: var(--vertical-tab-pf-color); |
If there are no other uses of --vertical-tab-pf-active-color
and --vertical-tab-pf-color
, you may be able to nix this stylesheet. Just a thought! That may be a horrible idea, since I really have no idea how this project is setup 😅
@@ -28,7 +28,7 @@ export const CatalogTileBadge: React.FunctionComponent<CatalogTileBadgeProps> = | |||
<Tooltip id={id} content={title}> | |||
<span className={classes} {...props}> | |||
{children} | |||
<span className="pf-v5-u-screen-reader">{title}</span> | |||
<span className="pf-v6-u-screen-reader">{title}</span> |
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.
Assuming these components import PF's base styles, any use of pf-v6-u-screen-reader
can be updated to use pf-v6-screen-reader
.
Two things and I think Coker caught one. Thanks for working on this Jessie!
|
@@ -26,44 +26,42 @@ import OkIcon from '@patternfly/react-icons/dist/esm/icons/ok-icon'; | |||
import ExternalLinkAltIcon from '@patternfly/react-icons/dist/esm/icons/external-link-alt-icon'; | |||
import GlobeIcon from '@patternfly/react-icons/dist/esm/icons/globe-icon'; | |||
|
|||
<div style={{ display: 'inline-block', padding: '15px', border: '1px solid grey' }}> |
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.
Took this wrapper off just so it's clear what style is coming from <PropertiesSidePanel>
and what's just in the example code.
@@ -118,7 +118,7 @@ class MockFilterSidePanelExample extends React.Component { | |||
const maxShowCount = 5; | |||
const leeway = 2; | |||
return ( | |||
<div style={{ width: '205px', border: '1px solid grey', paddingTop: '20px' }}> | |||
<div style={{ width: '205px' }}> |
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.
Removed the border and example padding so it's clear what's coming from <FilterSidePanel>
and what's part of the example. Left the width because there is no width on <FilterSidePanel>
and I'm assuming this panel goes into some parent/container with a constrained width, so this will mimic closer how the component will display. Ideally I'd probably drop this width, too, so you're just seeing the extension styles.
Just dropping a note here but this and the properties side panel seem to share some styling - they're narrow, probably have padding around them. And <FilterSidePanel>
has padding and no width, and <PropertiesSidePanel>
has a width and no padding. I'm guessing that's fine, but wondering if there could be some consistency there.
label="Certified Level" | ||
value={ | ||
<span> | ||
<OkIcon style={{color: '--pf-t--global--icon--color--status--success--default'}} /> Certified |
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.
updated this color token since the previous was just a generic success color and we have an icon specific success color.
<PropertiesSidePanel> | ||
<PropertyItem label="Operator Version" value="0.9.8 (latest)" /> | ||
<PropertyItem |
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.
Ideally this should just be a <DescriptionList>
. That's how it's called out in the figma design but I wasn't sure if that would work for the extension, depending on how it's used. If you used <DescriptionList>
there would be no custom styles here with maaayyyybe the exception of this width
react-catalog-view/packages/module/patternfly-docs/content/examples/propertiesSidePanel.css
Line 2 in d714e15
width: 165px; |
Everything else comes for free 💸
@@ -1,17 +1,13 @@ | |||
.ws-react-e-catalog-tile .catalog-tile-pf:active, .ws-react-e-catalog-tile .catalog-tile-pf:hover, .ws-react-e-catalog-tile .catalog-tile-pf:focus, .ws-react-e-catalog-tile .catalog-tile-pf:visited { | |||
color: inherit; |
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 seems a little weird. The link tile has blue text/stuff in it by default (since it's a link), but this style is making all of that black on :active, :hover, :focus, :visited
. So I just matched what I'm seeing in the browser for the current extension and updated the black color to use our default font color.
Here's the current extension - you'll likely need to update the href
on it to some non-visited link to see the default state (I updated to href="#"
)
and on hover
} | ||
.ws-react-e-catalog-tile .catalog-tile-pf-header .catalog-tile-pf-title { | ||
font-weight: 400; | ||
} | ||
.ws-react-e-catalog-tile .catalog-tile-pf-header .catalog-tile-pf-subtitle { |
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.
IMO this is a fair enhancement request for PF - a card subtitle. Then we could get rid of all of this, too.
.ws-react-e-catalog-tile .catalog-tile-pf-icon { | ||
font-size: 40px; | ||
height: 40px; | ||
font-size: 37px; |
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.
Updated to 37px per figma, but I don't know that it really matters if that needs to change to something else?
.ws-react-e-catalog-tile .catalog-tile-pf-badge-container { | ||
display: flex; | ||
flex: 1; | ||
justify-content: flex-end; | ||
margin-left: 10px; | ||
margin-inline-start: var(--pf-t--global--spacer--sm); | ||
} | ||
.ws-react-e-catalog-tile .catalog-tile-pf-badge-container .catalog-tile-pf-badge { | ||
font-size: 16px; | ||
margin-left: 5px; | ||
font-size: var(--pf-t--global--font--size--body--default); | ||
margin-inline-start: var(--pf-t--global--spacer--xs); | ||
} |
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'm not sure what all goes in this element but depending on what all it supports, this may be able to be improved.
color: #4CB140; | ||
color: var(--pf-t--global--icon--color--status--success--default); |
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 didn't see this in use, so maybe it can be removed?
@@ -1,50 +1,46 @@ | |||
.filter-panel-pf { |
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.
Ideally this whole thing would be a <Form>
. I think the markup would be different since currently each block of checkboxes is in its own <form>
HTML element but it should give exactly the styling needed. Here's a rough codesandbox https://codesandbox.io/p/sandbox/gallant-bouman-wcjx32?file=%2Findex.tsx%3A31%2C15. The margins and everything in the v6 <Form>
matched the figma design for this panel as far as I could tell.
} | ||
|
||
.filter-panel-pf-category-item .item-icon { | ||
padding-right: 3px; | ||
padding-inline-end: var(--pf-t--global--spacer--xs); |
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 seems like this spacer/padding and the one on .item-count
below could just come from regular space characters in the markup. The figma designs just use a space between the text and count.
@@ -1,21 +1,18 @@ | |||
.properties-side-panel-pf { |
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.
mentioned this above but all of the styles in this stylesheet come with the <DescriptionList>
component
@@ -32,22 +43,23 @@ | |||
text-overflow: ellipsis; | |||
white-space: nowrap; | |||
} | |||
.ws-react-e-vertical-tabs .vertical-tabs-pf-tab.active > a { | |||
color: var(--pf-t--global--text--color--brand--clicked); | |||
.ws-react-e-vertical-tabs .vertical-tabs-pf-tab > a.disabled { |
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 didn't see any examples of a disabled link in this extension, but it was in figma, so added support for <a class="disabled">
@@ -119,7 +119,7 @@ export class CatalogTile extends React.Component<CatalogTileProps> { | |||
> | |||
{(badges.length > 0 || iconImg || iconClass || icon || onClick) && ( | |||
<CardHeader | |||
actions={{ actions: badges.length > 0 && this.renderBadges(badges) }} | |||
actions={{ actions: badges.length > 0 && this.renderBadges(badges), hasNoOffset: true }} |
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 card actions element is positioned above where it would normally flow in the doc since the main use case here is for plain actions. The actions element moves itself up by the value of the top padding of a plain button, so that the top of the icon in that plain button is aligned with the top of the card header. If the actions element just has a normal thing in it (something other than an element with transparent padding on the top of it like a plain button), hasNoOffset
will position the actions element normally. You may want to adjust that depending on what all goes into the actions element of catalog tiles.
@jessiehuff @dlabaj pushed some updates! Discussed this with @jessiehuff but I'm not 100% these are the right stylesheets, though the ones I edited are what update the workspace styles, so I went ahead there. Some thoughts:
I'm not familiar with how this extension is used, where it goes in users' apps, what kinds of other/conflicting styles they may have in their app that some of the pre-existing extension styles may have been needed for. That said, I tried to leave most of styles in place that aren't needed to show the dev workspace examples properly, under the assumption they may be needed for other reasons. That said, I'd be more than happy to work with someone who could speak to how/where catalog view is used, and clean some of this stuff up. I don't suppose it's a big priority as there aren't all that many styles. I left a couple of comments about this but I think the properties side panel could just be updated to use |
Updated surge link: https://catalog-view-upgrade-v6.surge.sh/ |
🎉 This PR is included in version 6.0.0-alpha.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #51