-
Notifications
You must be signed in to change notification settings - Fork 17
Pro 8902 add styles #518
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
Pro 8902 add styles #518
Conversation
boutell
left a comment
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.
Great stuff, some notes but nothing huge I think
docs/.vitepress/sidebarGuide.js
Outdated
| text: 'Styling', | ||
| collapsed: true, | ||
| items: [ | ||
| { text: 'Global Styling', link: 'guide/global-styling.md' }, |
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.
Global Styles / Widget Styles seems better, I don't think it makes sense to have two variations on the word, just makes it harder to find later
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.
Changed
| @@ -0,0 +1,1098 @@ | |||
| # Global Styles | |||
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.
"Styles" here (which I think is correct), file should be renamed to match
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.
Changed.
docs/guide/global-styling.md
Outdated
|
|
||
| ## Configuration | ||
|
|
||
| Global styles are configured in a project-level `modules/@apostrophecms/styles/index.js` using a styles cascade that works much like the schema fields cascade. You define fields with types, labels, and properties—but instead of creating form inputs for content, these fields create controls for CSS properties. |
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.
Use emdash, spaces around the emdash
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.
Changed
docs/guide/global-styling.md
Outdated
|
|
||
| #### `unit` | ||
|
|
||
| Append units to numeric values from `range` or `box` fields. |
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.
Is it not supported for integer or float fields? If it's not then it's not...
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.
Since we are only recommending range, I'm not going to change this.
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 tested the integer and float fields. They are working so I updated this and added those fields in.
docs/guide/global-styling.md
Outdated
|
|
||
| // URL function | ||
| valueTemplate: 'url(%VALUE%)' | ||
| // Result: url(image.jpg) |
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 trying to picture a scenario where this second example would actually work out, but offhand I can't think of one. Maybe a select field with /modules image URLs? Or just skip it, not asking you to add more work here.
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 removed that example, but also updated the section for named object fields.
docs/guide/widget-styles.md
Outdated
| ### Performance considerations | ||
|
|
||
| - Widget styles are generated for each instance, so they add to page size | ||
| - For styles that should apply to all instances of a widget type, use regular CSS |
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.
use global styles or regular CSS
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
| - For styles that should apply to all instances of a widget type, use regular CSS | ||
| - Reserve widget styles for per-instance customization that content creators need | ||
|
|
||
| ## When to use widget styles vs. global styles |
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 whole section is great 🔥
| ``` | ||
|
|
||
| ::: warning | ||
| Widget modules do not support the `group` property in their `styles` configuration. Attempting to use grouping with widget styles will cause an error. |
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.
Good note
|
|
||
| ### `serverRendered` | ||
|
|
||
| When set to `true`, all CSS rendering (including preview during editing) goes through your custom `getStylesheet` method. This is required when implementing custom CSS generation logic. |
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 is available for global styles only. For performance reasons it is not available for widget-specific styles.
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.
Clarified.
docs/reference/modules/styles.md
Outdated
|
|
||
| The module automatically injects styles into every page: | ||
|
|
||
| - **For guests:** `<link>` tag pointing to cached stylesheet endpoint |
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.
Guests are logged-in users with a specific role. I suggest "for the public" or "for logged-out visitors."
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.
Changed.
docs/.vitepress/sidebarGuide.js
Outdated
| collapsed: true, | ||
| items: [ | ||
| { text: 'Global Styling', link: 'guide/global-styling.md' }, | ||
| { text: 'Global Styling', link: 'guide/global-styles.md' }, |
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.
Global Styles should be the title too, I don't see any reason to potentially confuse a developer on this point.
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.
Dumb oversight. Thanks.
docs/guide/global-styles.md
Outdated
|
|
||
| **Every field needs:** | ||
| - `type` (or `preset`) - The control type: `color`, `range`, `string`, `select`, `box`, or a preset name | ||
| - `type` (or `preset`) - The control type: `color`, `range`, `integr`, `float`, `string`, `select`, `box`, or a preset name |
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.
integr -> integer
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.
Fixed
docs/guide/widget-styles.md
Outdated
| - **Design Editors**: Grant both `edit` and `editStyles` permissions (can edit everything) | ||
|
|
||
| ::: warning | ||
| You must add `editPermission` to **every style field individually**. This includes fields within presets. The property cannot be set at the parent level and cascade down. |
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.
Per Miro we need to update this, how about:
You must add editPermission to every style field individually. In the case of presets, you may set editPermission once for each use of the preset and it will be applied to every field generated by that use of the preset.
| | **Delivery** | Cached stylesheet + inline | Inline per widget | | ||
| | **Interface** | Dedicated admin UI | Widget editor modal | | ||
| | **Selectors** | Required | Optional (for nested elements) | | ||
| | **Grouping** | Supports nested groups | Not supported | |
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 additional note is still needed:
| **Overrides** | Supports custom render method | Not supported |
docs/reference/modules/styles.md
Outdated
| ### `serverRendered` | ||
|
|
||
| When set to `true`, all CSS rendering (including preview during editing) goes through your custom `getStylesheet` method. This is required when implementing custom CSS generation logic. | ||
| **This option is only available for global styles.** Widget styles are always rendered server-side for performance reasons. |
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.
Not really, we render them on the front end when editing, which is one of the reasons for this policy.
This would be better language:
"Widget styles are always rendered by our standardized logic for performance reasons."
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.
Changed.
Summary
Summarize the changes briefly, including which issue/ticket this resolves. If it closes an existing Github issue, include "Closes #[issue number]"
This PR adds new documentation for global and per-widget styles. It also adds reference documentation for the styles module. Closes PRO-8902, PRO-8403, and PRO-8992.
What are the specific steps to test this change?
For example:
What kind of change does this PR introduce?
(Check at least one)
Make sure the PR fulfills these requirements:
If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.
Other information: