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

Add note and example about required css files for spacing classes #4297

Closed
wants to merge 1 commit into from

Conversation

ianw
Copy link

@ianw ianw commented Aug 16, 2021

It wasn't clear to me that the spacing.css is required to be explicitly imported to use these classes. Add a note on that.

@patternfly-build
Copy link

patternfly-build commented Aug 16, 2021

Preview: https://patternfly-pr-4297.surge.sh

CSS Size Report
NameCurrentPreviousDiff %
There are no changes in CSS file sizes

A11y report: https://patternfly-pr-4297-coverage.surge.sh

@mcoker
Copy link
Contributor

mcoker commented Aug 18, 2021

Hey @ianw thanks for the PR and thanks for exposing this hole in our docs! I talked with the PF team and we would like to explore how to:

  • Add this note globally to all of our utilities, in an automated way if possible, so that the wording and everything is consistent. Maybe via a HBS partial that accepts arguments for the parts of the blurb that are unique per utility? Or maybe just a generic blurb w/ example that can be used in all utilities?
  • Include docs on how to import in both HTML (<link rel="stylesheet" href="[path-to-pf]/utilities/Spacing/spacing.css">, <link rel="stylesheet" href="[path-to-pf]/patternfly-addons.css">) and react (import '@patternfly/react-styles/css/utilities/Spacing/spacing.css', import '@patternfly/react-styles/css/patternfly-addons.css')
  • Include the docs at the top of each utility. Maybe in a "default" alert, placed so that it works with and without the "beta feature" alert you'll see on beta components/layouts/utilities (here's an example - https://www.patternfly.org/v4/components/tree-view)

This will likely be an iterative process with a WIP PR for the team to review. Would you be interested in continuing this work to contribute that change?

@ianw
Copy link
Author

ianw commented Aug 19, 2021

* Add this note globally to all of our utilities, in an automated way if possible, so that the wording and everything is consistent. Maybe via a HBS partial that accepts arguments for the parts of the blurb that are unique per utility? Or maybe just a generic blurb w/ example that can be used in all utilities?

I think each of the "Utilities" listed requires their own css file, which isn't by default included? So it can't be generic i guess

* Include docs on how to import in both HTML (`<link rel="stylesheet" href="[path-to-pf]/utilities/Spacing/spacing.css">`, `<link rel="stylesheet" href="[path-to-pf]/patternfly-addons.css">`) and react (`import '@patternfly/react-styles/css/utilities/Spacing/spacing.css'`, `import '@patternfly/react-styles/css/patternfly-addons.css'`)

Is there an example of this to follow?

  * `patternfly-addons.css` includes all of the utilities - https://github.com/patternfly/patternfly/blob/main/src/patternfly/patternfly-addons.scss

There is a non-zero change we are doing this wrong ... we have

import '@patternfly/react-core/dist/styles/base.css'

is there a reason this can't also bring in these add-on CSS files? it seems sort of logical that everything would "just work" with that -- is it a performance issue?

* Include the docs at the top of each utility. Maybe in a "default" alert, placed so that it works with and without the "beta feature" alert you'll see on beta components/layouts/utilities (here's an example - https://www.patternfly.org/v4/components/tree-view)

Seems like this is defined in https://github.com/patternfly/patternfly-org? it probably seems a bit of overkill, i think just having in the documentation for each utility that it requires a specific css file would have helped

@stale
Copy link

stale bot commented Oct 18, 2021

This issue has been automatically marked as stale because it has not had activity in the last 60 days. It will be closed in 30 days if no further activity occurs.

@stale stale bot added the wontfix This will not be worked on label Oct 18, 2021
@stale stale bot closed this Nov 17, 2021
@dgdavid
Copy link
Contributor

dgdavid commented Feb 25, 2022

Although this was closed because of the lack of activity, documentation still missing a clue about this :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants