Skip to content
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

Merged
merged 19 commits into from
May 3, 2024
Merged

Conversation

jessiehuff
Copy link
Contributor

Closes #51

@andrew-ronaldson
Copy link

Here is a link to the Figma mocks.
Screenshot 2024-04-06 at 7 56 55 AM

Copy link
Contributor

@dlabaj dlabaj left a 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?

@dlabaj
Copy link
Contributor

dlabaj commented Apr 23, 2024

@jessiehuff Can you add a surge site to this PR?

@jessiehuff
Copy link
Contributor Author

Surge link for preview: https://catalog-view-v6.surge.sh/

Copy link
Contributor

@mcoker mcoker left a 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.

Screenshot 2024-04-23 at 8 01 44 PM

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
Copy link
Contributor

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

Suggested change
<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);
Copy link
Contributor

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

Suggested change
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines 1 to 2
$vertical-tab-pf-color: var(--pf-v6-global--Color--100) !default;
$vertical-tab-pf-active-color: var(--pf-v6-global--active-color--100) !default;
Copy link
Contributor

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

Suggested change
$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:

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

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>
Copy link
Contributor

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.

@andrew-ronaldson
Copy link

andrew-ronaldson commented Apr 25, 2024

Two things and I think Coker caught one. Thanks for working on this Jessie!

  1. The blue border-top should be removed
  2. The side filter panel that looks similar to our jump links component needs to be updated to match this mock.

@@ -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' }}>
Copy link
Contributor

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' }}>
Copy link
Contributor

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
Copy link
Contributor

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.

Comment on lines +29 to +31
<PropertiesSidePanel>
<PropertyItem label="Operator Version" value="0.9.8 (latest)" />
<PropertyItem
Copy link
Contributor

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

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;
Copy link
Contributor

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="#")

Screenshot 2024-05-02 at 5 45 33 PM

and on hover

Screenshot 2024-05-02 at 5 45 45 PM

}
.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 {
Copy link
Contributor

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;
Copy link
Contributor

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?

Comment on lines 26 to 35
.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);
}
Copy link
Contributor

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.

Comment on lines -53 to +46
color: #4CB140;
color: var(--pf-t--global--icon--color--status--success--default);
Copy link
Contributor

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 {
Copy link
Contributor

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);
Copy link
Contributor

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 {
Copy link
Contributor

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 {
Copy link
Contributor

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 }}
Copy link
Contributor

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.

@mcoker
Copy link
Contributor

mcoker commented May 3, 2024

@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 <DescriptionList> under the hood and likely wouldn't need any styles/overrides or anything to be kept in sync with PF going forward, which would be awesome. Same for filter side panel - that looks like an implementation of the <Form> component to me. I didn't make that decision/update in this PR though, but I'm happy to either do that or help out with it if we do want to make that update.

@jessiehuff
Copy link
Contributor Author

Updated surge link: https://catalog-view-upgrade-v6.surge.sh/

@dlabaj dlabaj merged commit 5b2d6e0 into patternfly:v6 May 3, 2024
4 checks passed
Copy link

🎉 This PR is included in version 6.0.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

catalog-view: Consume v6 alphas and build Penta demo
4 participants