-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[admin] Document SolidusAdmin intended usage and how to contribute #5595
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5595 +/- ##
=======================================
Coverage 88.56% 88.56%
=======================================
Files 685 685
Lines 16408 16408
=======================================
Hits 14531 14531
Misses 1877 1877 ☔ View full report in Codecov by Sentry. |
admin/docs/tailwindcss.md
Outdated
|
||
The Tailwind CSS stylesheets are precompiled and included in the gem. | ||
This means that the gem can be used without having to compile Tailwind CSS in the host application. | ||
The compiled file is not included in the git repository, but it is generated and included in the gem as |
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.
What do you think about committing the built tailwind css file into the admin repo (and update it on PR merge)? That file won't change much (at least once the admin has been built out). And even until then it makes sense to have it updated regularly (not just on gem release).
People might use solidus_admin from git source like this
# Gemfile
gem "solidus_admin", github: "solidusio/solidus", branch: "v4.3"
(in order to get bug fixes early for stable versions that have not been released yet.)
We want to make sure they get the admin css file, right?
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 have a GH workflow that builds a JS bundle for Alchemy CMS on every merge (and when the source files have changed) https://github.com/AlchemyCMS/alchemy_cms/blob/main/.github/workflows/build.yml
I am happy to contribute this to solidus if we agree on adding the file into the repo
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.
@tvdeyen I really didn't want to add such a noisy file, but I admin it might be the easiest thing to do. Do you usually update it manually in Alchemy and then use the GH action as a fallback or you rely on it as the main thing that updates the file?
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 really didn't want to add such a noisy file
I won't consider this file noise (at least until the new admin is done). What is the difference to other assets we have committed into our repo?
Do you usually update it manually in Alchemy and then use the GH action as a fallback or you rely on it as the main thing that updates the file?
I manually update the bundle whenever I add a dependency. The GH action is used for automated dependency updates (like dependabot or depfu)
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.
Today we discussed this topic during the core team meeting, and the consensus seems to be to try to avoid directly committing the generated CSS artifact file to this repository. One interesting option that came up is replacing tailwindCSS with https://unocss.dev/, which should remove the issue. I'll try to investigate more on 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.
@tvdeyen I'm thinking that regardless of the solution we'll be able to find for the sandbox issue, this PR should be progressed since it includes documentation on many other topics and, speaking of the problematic new admin tailwind.css
, it documents the current behavior, which otherwise would remain rather unexposed. We have #5598 for tracking the sandbox issue and exploring its solutions.
Since the gem has specific releases, it makes sense to point to its specific readme, code path and changelog URL. Co-Authored-By: Elia Schito <[email protected]> Co-Authored-By: Rainer Dema <[email protected]>
A few new sections are added, while other are edited in order to better describe the current state of the new admin. Co-Authored-By: Elia Schito <[email protected]> Co-Authored-By: Rainer Dema <[email protected]>
2973d57
to
c758304
Compare
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.
✅ Leaving a sort of approval for the changes made
@tvdeyen I merged giving the above conversation proper space in an issue, after all this documentation accurately describes the current status of things, if we switch to a different approach we'll update it accordingly. |
fine for me. |
Summary
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: