-
Notifications
You must be signed in to change notification settings - Fork 41
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
PWA-2915: Added Tailwind CSS configuration and theming documentation #155
Conversation
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 think it's premature to change the theme. Theme changes shouldn't be made in a content branch anyway. Please back out those changes, and we'll talk about the benefits and repercussions of changing the theme at our next meeting.
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 was fooled by the intro/step 1, thinking the topic is of limited use for those who installed with the CLI. As a result, some of my suggestions maybe aren't applicable. I'm not going to delete/amend them, because you should know what a reader could experience.
src/pages/guides/theming/index.md
Outdated
|
||
# Theming PWA Studio apps | ||
|
||
When creating a new PWA Studio app with [@magento/create-pwa](https://www.npmjs.com/package/@magento/create-pwa), theming is configured out of the box to use Tailwind CSS. The CLI installs all the supporting Tailwind CSS packages and configuration files described below. For a PWA Studio app that was not scaffolded with the CLI, this topic describes how to configure your app for theming with Tailwind 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.
Consider specifying the exact command in the second sentence.
Start a new paragraph with the sentence that begins with "For a". Maybe even make it a note. People skim intros, and we don't want someone who ran the command be under the impression they also have to do all these steps. Also consider stating CLI users should do next, since there's nothing else to do here.
src/pages/guides/theming/index.md
Outdated
After installation, your `package.json` file should contain the following entries: | ||
|
||
```json | ||
// package.json |
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 causing all sorts of GH rendering problems. It's not necessary, since you listed it in the lead-in sentence.
src/pages/guides/theming/index.md
Outdated
|
||
## Step 3: Test your configuration | ||
|
||
To test your configuration with the `[venia]` theme set as a `preset` in your config file, add the following `theme` style override and rebuild your app. |
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.
At the beginning of this topic, you described that it was meant for those who didn't use the default scaffolding. I was willing to give the benefit of the doubt that step 2 was just for setting up in an alternate environment, but really, it does contain advice for changing the defaults. As of this step though, it's clear that it applies to everyone.
Rewrite the intro and make sure it's clear that
- Step 1 provides details about the two installation paths
- Alternate scaffolding users must go through step 2, but the happy path installers might need to make changes from the default configuration
- Steps 3 and 4 are for everyone.
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.
Below, in option 2, you say modifying the config.js
file isn't the right thing to do in most cases. I recommend saying that this step is a quick test, and you might want to back out these changes when creating your own presets.
src/pages/guides/theming/index.md
Outdated
|
||
### Option 1: Update tailwind.config.js theme directly | ||
|
||
If you followed Step 3 above, you have already done this as a test. Another example: change the default font family by adding new styles to the `tailwind.config.js` `theme` property: |
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 would assume that the reader followed step 3 (no need to write "above"). Add the fontFamily example below to step 3, and use this section point out this is what you did, but you probably don't want to use it in a real-world scenario.
src/pages/guides/theming/index.md
Outdated
|
||
### Override or extend Tailwind base styles | ||
|
||
To override or extend the Tailwind base styles for version `2.2.19`, and any other version, use this site as a [reference to the Tailwind base styles](https://unpkg.com/browse/tailwindcss@2.2.19/stubs/defaultConfig.stub.js). |
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.
Can you say something like "the base styles for any supported version of TW"? Again, trying to future-proof the docs.
I agree. This was a test that I should have mentioned in the PR description. Until this, I've never tried applying an npm dependency on the commerce-theme outside the project's internal |
Co-authored-by: Kevin Harper <keharper@users.noreply.github.com>
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.
Looking much better. You can accept/reject my suggestions.
Co-authored-by: Kevin Harper <keharper@users.noreply.github.com>
@keharper They look good to me. Thanks. |
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.
just one minor suggestion, but looks good otherwise
Co-authored-by: James Calcaben <jcalcaben@users.noreply.github.com>
Description
This PR adds documentation for configuring and customizing Tailwind and Venia themes.
Staged Preview
https://developer-stage.adobe.com/commerce/pwa-studio/guides/theming
Related Issue
PWA-2915
Motivation and Context
Provide docs for existing Tailwind CSS theming in PWA Studio.
How Has This Been Tested?
Created a PWA Studio app from the
create-pwa
CLI and applied the examples to it to see the expected results.Screenshots (if appropriate):
Types of changes