fix(styles): Replace workspace styles with content classes by wise-king-sullyman · Pull Request #4011 · patternfly/patternfly-org

wise-king-sullyman · 2024-05-08T17:36:34Z

patternfly-build · 2024-05-08T17:44:27Z

Preview: https://patternfly-org-pr-4011-site.surge.sh

mcoker · 2024-05-13T15:54:42Z

packages/documentation-framework/templates/mdx.css

-/*
- * Copied from pf-v6-c-content.
- */
-:root {
-  --pf-v6-c-content--MarginBottom: var(--pf-t--global--spacer--md);
-/*   --pf-v6-c-content--LineHeight: var(--pf-v6-global--LineHeight--md); */


mcoker

Left some commentary from browsing the files changed since it's still in draft.

mcoker · 2024-05-13T19:08:10Z

packages/documentation-framework/templates/mdx.css

 }

-.ws-p + .ws-code {
+.pf-v6-c-content--p + .ws-code {


Ideally you shouldn't need this anymore. If you use .pf-v6-c-content--p on an element and anything comes after it, .pf-v6-c-content--p will include its default paragraph margin.

mcoker · 2024-05-13T19:10:33Z

packages/documentation-framework/templates/mdx.js

        <TableOfContents items={toc} />
      )}
-      <div className="ws-mdx-content">
+      <div className="pf-v6-c-content">


Probably don't want this class as a wrapper unless basically everything inside of it is just HTML like markdown might generate. This will tell any of the elements the text/content component supports (<p>, <ul>, etc) to get default styling.

Using the .pf-v6-c-content--[element] classes directly on elements you want to receive the styling removes the need for this wrapper.

mcoker · 2024-05-13T19:31:31Z

packages/documentation-framework/components/autoLinkHeader/autoLinkHeader.css

@@ -1,8 +1,8 @@
-.ws-heading {
+.pf-v6-c-content--heading {


IMO these .ws-[...] classes for the anchor styles are OK as-is. Since we don't have these classes in the content component, I think it might be confusing if we add/style them in the org CSS

mcoker · 2024-05-13T20:31:19Z

...ation-site/patternfly-docs/content/design-guidelines/styles/colors-for-charts/colors-grid.js

  return (
    <GridItem>
-      <h3 className="pf-v6-c-title pf-m-xl ws-heading ws-title ws-h3">{color} family</h3>
+      <h3 className="pf-v6-c-title pf-m-xl pf-v6-c-content--heading ws-title pf-v6-c-content--h3">{color} family</h3>


This may be known, but we don't currently have a .pf-v6-c-content--heading class.

mcoker · 2024-05-13T20:34:40Z

...s/documentation-site/patternfly-docs/content/design-guidelines/styles/colors/ColorSwatch.css

@@ -1,17 +1,17 @@
 .ws-color-swatch {


We may want to review this stylesheet and see if there is anything we can do differently. Though with these existing styles, we'll want to use --pf-t tokens instead of pf-v6-global vars, and potentially a few other tweaks - tokens for border-radius, font-weight, potentially the 44px flex-basis on the <svg> converted to an icon size token.

mcoker · 2024-05-13T20:36:10Z

packages/documentation-site/patternfly-docs/content/design-guidelines/styles/icons/icons.css

@@ -1,8 +1,8 @@
-.pf-v5-c-divider.ws-icons-divider {


This stylesheet is another good one to review as a whole and see if we can't use some existing components/layouts/styles. With the styles here, one big one that sticks out is we're not using spacer vars for spacers we support.

Made a followup here: #4020

mcoker · 2024-05-13T20:38:32Z

packages/documentation-site/patternfly-docs/content/get-started/community/community.md

  </Grid>

-  <Title size="3xl" className="pf-v5-u-mb-sm ws-page-title pf-v5-u-mt-3xl" headingLevel="h2">Creating new communities</Title>
+  <Title size="3xl" className="pf-v6-u-mb-sm ws-page-title pf-v6-u-mt-3xl" headingLevel="h2">Creating new communities</Title>


I don't know how much it matters, but it's worth noting there are a lot of utility classes used on this page where it looks like we have a stylesheet for other pages? I wonder if we should be more consistent. Also I wonder if we used <Text> headings, if we would need all of these margins and hwhat-not on the headings.

Followup #4021

mcoker · 2024-05-13T20:39:19Z

packages/documentation-site/patternfly-docs/content/get-started/community/community.md

  <p>The PatternFly community is never finished growing, and we want to keep it that way. Feel free to reach out whenever — we're always open.</p>

-  <Grid sm={12} md={6} gutter="sm" className="pf-v5-u-my-lg pf-v5-l-grid pf-m-all-12-col-on-sm pf-m-all-6-col-on-md pf-m-gutter" style={{ maxWidth: '450px' }}>
+  <Grid sm={12} md={6} gutter="sm" className="pf-v6-u-my-lg pf-v6-l-grid pf-m-all-12-col-on-sm pf-m-all-6-col-on-md pf-m-gutter" style={{ maxWidth: '450px' }}>


I'd expect we have <Grid> props that can replace the use of these .pf-v6-l-grid/.pf-m-* classes?

Followup #4021

mcoker · 2024-05-13T20:41:04Z

packages/documentation-site/patternfly-docs/content/training/trainingCard/trainingCard.css

@@ -1,18 +1,18 @@
-.pf-v5-c-card:not(.pf-org__card-small).pf-org__card {


I wonder if we could get rid of some of these styles. If not, we should probably tokenify these styles.

Followup #4023

mcoker

This is awesome! Left some comments, but a lot are kind of open ended and might need more work, so not sure if they need to be addressed at all or in this PR.

A couple of extras that didn't pertain to a line of code:

Under the props docs, a component name above a props table has an "undefined" class in its classlist. That's in v5, too, but figured I'd mention it.

We don't have support in the text/content component for all of these items in styledMdTags. I'm not sure if that's a problem? Particularly <code>, <table>, and <img>. Also ul is in there thrice and we have dl and dt but no dd

patternfly-org/packages/documentation-framework/scripts/md/styled-tags.js

Lines 3 to 18 in b596b2a

    
           const styledMdTags = [ 
        
             'p', 
        
             'ul', 
        
             'ul', 
        
             'ul', 
        
             'ol', 
        
             'li', 
        
             'dl', 
        
             'blockquote', 
        
             'small', 
        
             'hr', 
        
             'dt', 
        
             'code', 
        
             'table', 
        
             'img' 
        
           ];

We maayyy consider keeping the ws-[el] classes on the markdown generated elements just so we have a way to target them with styles or whatever uniquely and not risk styling .pf-v6-c-content--whatever for docs-framework and have that impact the styling of .pf-v6-c-content--whatever in our component examples (which is a bug that exists currently)

mcoker · 2024-05-16T22:25:01Z

packages/documentation-framework/templates/mdx.js

          {functionDocumentation.length > 0 && (
            <React.Fragment>
-              <AutoLinkHeader size="h2" className="ws-h2" id="functions">
+              <AutoLinkHeader size="h2" className="pf-v6-c-content--h2" id="functions">


<AutoLinkHeader> uses a <Title> under the hood. That's basically assigning 2 heading component classes to it and could have conflicting styles. I wonder if we could just use the <Text> component in <AutoLinkHeader> instead of <Title>? It would really simplify this and the autolinkheader code.

mcoker · 2024-05-17T00:42:02Z

...ation-site/patternfly-docs/content/design-guidelines/styles/colors-for-charts/colors-grid.js

  return (
    <GridItem>
-      <h3 className="pf-v6-c-title pf-m-xl ws-heading ws-title ws-h3">{color} family</h3>
+      <h3 className="pf-v6-c-title pf-m-xl ws-heading ws-title pf-v6-c-content--h3">{color} family</h3>


Same here as the <AutoLinkHeader> - I wonder if we can just use <Text> (.pf-v6-c-content--h3) and remove <Title>? Also this has .ws-heading and .ws-title - looks like neither are necessary. I don't see any styles for .ws-title - do you know if there is a style defined for that somewhere?

mcoker · 2024-05-17T00:50:41Z

...s/documentation-site/patternfly-docs/content/design-guidelines/styles/colors/ColorSwatch.css

 .ws-color-swatch-description-label {
-  font-size: var(--pf-v5-global--FontSize--xs);
+  font-size: var(--pf-t--global--font--size--xs);
  font-weight: bold;


Anywhere we have bold regular (body, aka non-heading) text, we'll need to use var(--pf-t--global--font--weight--body--bold). FWIW that's because the browser resolves the word "bold" as 700, and in our variable font, the "bold" weight is 500.

mcoker · 2024-05-17T14:54:33Z

packages/documentation-site/patternfly-docs/content/design-guidelines/styles/icons/icons.css

@@ -1,8 +1,8 @@
-.pf-v5-c-divider.ws-icons-divider {
+.pf-v6-c-divider.ws-icons-divider {
  margin: 48px 0;


I'm not sure where this is used, but this is adding a pretty big top/bottom margin. The <Text> (and all the other <Text*>) component(s) do not currently support a divider (for spacing/styling) but the core component does (it supports <hr> or <hr class="pf-v6-c-content--hr">. If we add that support to the react component, I wonder if we could get rid of these custom divider styles. Or possibly do something like use a <Divider> between page sections, so the top/bottom padding on the page sections provide space for the divider between the sections.

But ideally we'd use a spacer here if we keep this style. A 48px spacer is --pf-t--global--spacer--2xl

Also I just glanced at a couple of pages and noticed we have a colors page divider with custom margin, too - if they're similar, it'd be super cool if they just used the same styling that ideally came for free.

React followup patternfly/patternfly-react#10390

Org followup: #4033

mcoker · 2024-05-17T14:56:04Z

packages/documentation-site/patternfly-docs/content/design-guidelines/styles/icons/icons.css


-.pf-v5-c-divider.ws-icons-divider:first-of-type {
+.pf-v6-c-divider.ws-icons-divider:first-of-type {
  margin-top:8px;


Same comment here - I wonder if there is a way we can just remove this style. But 8px is also --pf-t--global--spacer--sm

mcoker · 2024-05-17T14:58:09Z

packages/documentation-site/patternfly-docs/content/design-guidelines/styles/icons/icons.css


-#all-icons ~ .ws-p > .ws-code {
+#all-icons ~ .pf-v6-c-content--p > .ws-code {
  margin-left: 8px;


Just gonna sneak this in here, but all of these overrides are also not using logical properties, so RTL won't work on org - which is fine, AFAIK that's not a goal or anything, just a nice to have I reckon.

Suggested change

margin-left: 8px;

margin-inline-start: var(--pf-t--global--spacer--sm);

mcoker · 2024-05-17T15:00:25Z

packages/documentation-site/patternfly-docs/content/design-guidelines/styles/icons/icons.css


-.ws-icons-gridtext .ws-icon-sizes .ws-p {
+.ws-icons-gridtext .ws-icon-sizes .pf-v6-c-content--p {
  margin-bottom: 8px;


This should come with default paragraph margin so I wonder if we can remove this style and live with the spacing we get for free from .pf-v6-c-content--p

mcoker

Super small nits in spacer tokens. Hopefully I got 'em all?

mcoker · 2024-05-21T18:06:03Z

...s/documentation-site/patternfly-docs/content/design-guidelines/styles/colors/ColorSwatch.css

 .ws-color-swatch {
  display: flex;
-  margin-top: var(--pf-v5-global--spacer--md);
+  margin-top: var(--pf-t--global--spacer-md);


Suggested change

margin-top: var(--pf-t--global--spacer-md);

margin-top: var(--pf-t--global--spacer--md);

mcoker · 2024-05-21T18:06:12Z

...s/documentation-site/patternfly-docs/content/design-guidelines/styles/colors/ColorSwatch.css

 .ws-color-swatch > svg {
  flex: 0 0 44px;
-  margin-right: var(--pf-v5-global--spacer--md);
+  margin-right: var(--pf-t--global--spacer-md);


Suggested change

margin-right: var(--pf-t--global--spacer-md);

margin-right: var(--pf-t--global--spacer--md);

mcoker · 2024-05-21T18:07:08Z

...s/documentation-site/patternfly-docs/content/design-guidelines/styles/colors/ColorSwatch.css

  display: inline-block;
-  padding-top: var(--pf-v5-global--spacer--md);
-  padding-bottom: var(--pf-v5-global--spacer--xs);
+  padding-top: var(--pf-t--global--spacer-md);


Suggested change

padding-top: var(--pf-t--global--spacer-md);

padding-top: var(--pf-t--global--spacer--md);

mcoker · 2024-05-21T18:07:21Z

...s/documentation-site/patternfly-docs/content/design-guidelines/styles/colors/ColorSwatch.css

 .ws-color-swatch-popover-code {
  white-space: nowrap;
-  margin-bottom: var(--pf-v5-global--spacer--md);
+  margin-bottom: var(--pf-t--global--spacer-md);


Suggested change

margin-bottom: var(--pf-t--global--spacer-md);

margin-bottom: var(--pf-t--global--spacer--md);

jessiehuff

Don't want to hold up this PR, but I think we probably should look into the spacing in a followup. It feels really tight in some spots, especially in the a11y docs for each component.

fix(styles): Replace workspace styles with content classes

8cee64f

kmcfaul linked an issue May 9, 2024 that may be closed by this pull request

Identify & resolve any custom styles #3945

Closed

mcoker reviewed May 13, 2024

View reviewed changes

wise-king-sullyman added 2 commits May 14, 2024 14:00

fix(styles): revert content classes to ws where needed, replace globals

91e5638

Merge branch 'v6' into resolve-custom-styles-pf6

b596b2a

wise-king-sullyman marked this pull request as ready for review May 14, 2024 18:14

wise-king-sullyman requested review from jessiehuff and mcoker May 15, 2024 13:30

mcoker reviewed May 17, 2024

View reviewed changes

mcoker mentioned this pull request May 20, 2024

Text - support for hr element patternfly/patternfly-react#10390

Closed

Merge branch 'v6' into resolve-custom-styles-pf6

c6d2a9f

wise-king-sullyman mentioned this pull request May 21, 2024

Remove custom divider styles in icons.css #4033

Closed

chore(): Style and classname changes per review comments

402c341

mcoker approved these changes May 21, 2024

View reviewed changes

chore(styles): Fix token names

b783934

jessiehuff approved these changes May 21, 2024

View reviewed changes

This was referenced May 21, 2024

Update AutoLinkHeader to use the Text component rather than Title #4035

Closed

Update colors-grid headings to use Text component #4036

Closed

Accessibility page spacing is wrong in v6 #4037

Closed

jessiehuff merged commit f6faa02 into patternfly:v6 May 21, 2024

wise-king-sullyman deleted the resolve-custom-styles-pf6 branch May 21, 2024 20:40

		@@ -1,18 +1,18 @@
		.pf-v5-c-card:not(.pf-org__card-small).pf-org__card {

	const styledMdTags = [
	'p',
	'ul',
	'ul',
	'ul',
	'ol',
	'li',
	'dl',
	'blockquote',
	'small',
	'hr',
	'dt',
	'code',
	'table',
	'img'
	];

	margin-left: 8px;
	margin-inline-start: var(--pf-t--global--spacer--sm);

	margin-top: var(--pf-t--global--spacer-md);
	margin-top: var(--pf-t--global--spacer--md);

	margin-right: var(--pf-t--global--spacer-md);
	margin-right: var(--pf-t--global--spacer--md);

	padding-top: var(--pf-t--global--spacer-md);
	padding-top: var(--pf-t--global--spacer--md);

	margin-bottom: var(--pf-t--global--spacer-md);
	margin-bottom: var(--pf-t--global--spacer--md);

		@@ -1,8 +1,8 @@
		.ws-heading {
		.pf-v6-c-content--heading {

Conversation

wise-king-sullyman commented May 8, 2024

Uh oh!

patternfly-build commented May 8, 2024 • edited 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!

mcoker 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!

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!

mcoker 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!

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!

mcoker 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!

jessiehuff left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

patternfly-build commented May 8, 2024 •

edited

Loading