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

enable tailwind nesting #29746

Merged
merged 7 commits into from
Mar 14, 2024
Merged

Conversation

rafh
Copy link
Contributor

@rafh rafh commented Mar 12, 2024

Currently, if you implement native CSS nesting within a Vue component a warning will appear in the terminal. It states
Nested CSS was detected, but CSS nesting has not been configured correctly. Please enable a CSS nesting plugin *before* Tailwind in your configuration. To fix this error we need to enable the built-in tailwinds nesting config.

Example code to trigger the warning within a vue component:

<style>
.example {
  &:hover,
  &:focus-visible {
    color: var(--color-text);
  }

  & svg {
    margin-right: 0.78rem;
  }
}
</style>

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 12, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 12, 2024
@silverwind
Copy link
Member

silverwind commented Mar 12, 2024

Won't this also get us postcss-nested? Seems like a nonstandard nesting syntax I would like to avoid.

Generally I'm fine with nesting syntax currently if it's standard syntax and when it's compiled to flat syntax for compat.

@jtran
Copy link
Contributor

jtran commented Mar 12, 2024

We already have postcss-nested in our dependencies.

"postcss-nested": "^6.0.1",

@silverwind, would you prefer to use postcss-nesting instead which uses the standard?

@silverwind
Copy link
Member

silverwind commented Mar 12, 2024

Yeah, it's a dependency of tailwindcss, but my preference would be standard nesting syntax, transpiled to flat syntax until PaleMoon gains support, or just not use it until then 😆.

@silverwind
Copy link
Member

silverwind commented Mar 12, 2024

Seems https://github.com/csstools/postcss-plugins/tree/main/plugins/postcss-nesting, so please add that as dependency and make it use that, also set edition to latest, which will make removing the plugin later easier.

Copy link
Contributor

@jtran jtran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

package.json Outdated Show resolved Hide resolved
@silverwind
Copy link
Member

silverwind commented Mar 13, 2024

Yes looks good besides that topic above. I will also give it a test later.

@silverwind
Copy link
Member

silverwind commented Mar 13, 2024

One final question: What is the motivation for this PR? We don't have any CSS nesting in the code yet as far as I'm aware. Is it for customization or are you preparing a PR that will use it?

@rafh
Copy link
Contributor Author

rafh commented Mar 14, 2024

One final question: What is the motivation for this PR? We don't have any CSS nesting in the code yet as far as I'm aware. Is it for customization or are you preparing a PR that will use it?

We use CSS nesting within our fork. I thought this would be a good candidate to push upstream because others may eventually use nesting in their PRs.

@silverwind
Copy link
Member

Ok, personally I wouldn't use it, but we can give people this option.

@rafh rafh requested review from jtran and silverwind March 14, 2024 15:49
Copy link
Contributor

@jtran jtran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 14, 2024
@silverwind
Copy link
Member

silverwind commented Mar 14, 2024

Tested it: In Vue components I see the CSS is flattened as expected. But if I edit a regular CSS file it isn't:

image

Not sure what's the issue, but I would expect CSS to be flattened in both cases. Example CSS:

.page-footer {
  .left-links {
    display: flex;
    flex-wrap: wrap;
    align-items: center;
    justify-content: center;
    gap: 0.25em;
    color: red;
  }
}

@silverwind
Copy link
Member

Tested it: In Vue components I see the CSS is flattened as expected. But if I edit a regular CSS file it isn't

Fixed in 9a7482f. The magic option importLoaders: 1 made it work.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 14, 2024
@silverwind silverwind merged commit 03753cb into go-gitea:main Mar 14, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Mar 14, 2024
@rafh rafh deleted the add-tailwind-nesting-config branch March 15, 2024 00:26
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 15, 2024
* giteaofficial/main:
  Update JS dependences (go-gitea#29797)
  Unify search boxes (go-gitea#29530)
  Fix document error about 'make trans-copy' (go-gitea#29710)
  Remove jQuery AJAX from the diff functions (go-gitea#29743)
  Fix Safari spinner rendering (go-gitea#29801)
  Remove jQuery AJAX from the `repo-issue.js` file (go-gitea#29776)
  Improve commit record's ui in comment list (go-gitea#26619)
  enable tailwind nesting (go-gitea#29746)
@6543 6543 modified the milestones: 1.23.0, 1.22.0 Mar 16, 2024
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jun 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/dependencies modifies/js size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants