patternfly-elements icon indicating copy to clipboard operation
patternfly-elements copied to clipboard

[bug][docs] Fix numerous docs style issues

Open markcaron opened this issue 2 years ago • 7 comments

There are multiple style issues throughout PFE doc pages:

  • [x] Add new site header style (@markcaron — #2396)
    • [x] Add new PFE logo https://github.com/patternfly/patternfly-elements/pull/2401
    • [x] ~~Fix v1/v2 toggle to be consistent in style~~ (see comment below)
    • [x] ~~Have v1/v2 toggle go directly to All Components (not individual components)~~ (see comment below)
  • [x] Page headings
    • [x] Add back in gray background behind page headings https://github.com/patternfly/patternfly-elements/pull/2398
    • [x] ~~Move overview description into page heading~~ (see comment below)
  • [x] Add max-width to content https://github.com/patternfly/patternfly-elements/pull/2398
  • [x] Bands
    • [x] ~~Fix spacing between "bands"~~ (see comment below)
    • [x] Remove extra <p> between bands (#2419) https://github.com/patternfly/patternfly-elements/pull/2421
  • [ ] Components
    • [x] Accordion
      • [x] API content/sections doesn't match PF (should mimic) https://github.com/patternfly/patternfly-elements/pull/2417
    • [ ] Clipboard copy
      • [ ] Icon doesn't match PatternFly's (see comment below)
      • [x] Icon size https://github.com/patternfly/patternfly-elements/pull/2418
      • [x] Bottom border doesn't lineup is both versions https://github.com/patternfly/patternfly-elements/pull/2418
  • [ ] Font changes on in-page demos, like accordion's constructed stylesheet (i.e., "Red Hat Text" vs "RedHatTextUpdated") #2427
  • [x] Get Started theming example uses cornflowerblue which seems to be a poor color choice. #2409
  • [X] Blog fixes Fixed in #2459
  • [x] Fix favicon Fixed in #2456
  • [ ] Fix "Get Started" CTA on homepage (@eyevana in progress) https://github.com/patternfly/patternfly-elements/pull/2457
  • [x] Accordion page focus bug PR https://github.com/patternfly/patternfly-elements/pull/2458

markcaron avatar Feb 28 '23 19:02 markcaron

  • [ ] Fix v1/v2 toggle to be consistent in style
  • [ ] Have v1/v2 toggle go directly to All Components (not individual components)

I believe we are deferring these due to symlinking to the v1 docs and not directly maintaining their templates.

zeroedin avatar Mar 10 '23 15:03 zeroedin

  • [ ] Move overview description into page heading

Due to the usage of pfe-tools to render these pages we can't modify these without a breaking change to child repos.

zeroedin avatar Mar 10 '23 16:03 zeroedin

  • [ ] Icon doesn't match PatternFly's

The difference is between font awesome version 6 and version 5

https://fontawesome.com/v6/icons/copy?s=solid&f=classic

https://fontawesome.com/v5/icons/copy?s=solid&f=classic

@bennypowers was this the intent of pf-icon to use version 6 of fontawesome rather then version 5?

zeroedin avatar Mar 10 '23 16:03 zeroedin

  • [ ] Icon size

The icon sizes are not the same as PFv4 clipboard. Currently, the copy icon is showing up at 10px which is the default size of a <pf-icon> with no-size variant given (eg. size="sm|md|lg").

pf-icon size API does not allow for a 16px icon, even though this is the size that PFv4 itself uses for many of its components, this issue has come up in several places including pf-tabs and downstream in RHDS. You can override this with CSS prop --pf-icon--size: 16px.

Patternfly icon states this same intent of 10px = sm,18px = md, 24px = lg, 54px = xl. Even though use cases throughout PFv4 of a 16px size <svg> are used in components for many icons.

The reason this is being exposed is that these components are not directly using the PF <Icon/> react component like we are reusing the <pf-icon> in our web components. This is the discrepancy that is causing the size issue differences.

zeroedin avatar Mar 10 '23 16:03 zeroedin

[ ] Font changes on in-page demos, like accordion's constructed stylesheet (i.e., "Red Hat Text" vs "RedHatTextUpdated")

@brianferry will you take a look at this one with Accordion? I'm pretty sure the solution here is just to remove "RedHatTextUpdated" and replace it with "Red Hat Text" as we won't serve "RedHatTextUpdated" on any of the sites this gets used on. If I understand the issue correctly. I believe @markcaron can expand on that if needed.

zeroedin avatar Mar 10 '23 19:03 zeroedin

@zeroedin No need to adjust the band padding. I marked off the item from the checklist above. But, I added a item below it to remove the extra <p> between bands.

markcaron avatar Mar 10 '23 19:03 markcaron

@markcaron @hellogreg please take a quick look and see if we can maybe close this and open some new, targeted issues on the PFE milestone

bennypowers avatar Oct 18 '23 05:10 bennypowers