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

[admin] Document SolidusAdmin intended usage and how to contribute #5595

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

elia
Copy link
Member

@elia elia commented Jan 15, 2024

Summary

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

Copy link

codecov bot commented Jan 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4b46992) 88.56% compared to head (c758304) 88.56%.

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.
📢 Have feedback on the report? Share it here.


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
Copy link
Member

@tvdeyen tvdeyen Jan 17, 2024

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?

Copy link
Member

@tvdeyen tvdeyen Jan 17, 2024

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

Copy link
Member Author

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?

Copy link
Member

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)

Copy link
Member

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.

Copy link
Member

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.

spaghetticode and others added 2 commits January 31, 2024 18:10
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]>
@spaghetticode spaghetticode force-pushed the elia+rainer/admin/contributing branch from 2973d57 to c758304 Compare January 31, 2024 17:13
@spaghetticode spaghetticode marked this pull request as ready for review February 1, 2024 11:54
@spaghetticode spaghetticode requested a review from a team as a code owner February 1, 2024 11:54
Copy link
Member Author

@elia elia left a 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

@elia elia merged commit 020f3c2 into main Feb 8, 2024
14 checks passed
@elia elia deleted the elia+rainer/admin/contributing branch February 8, 2024 10:46
@elia
Copy link
Member Author

elia commented Feb 8, 2024

@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.

@tvdeyen
Copy link
Member

tvdeyen commented Feb 8, 2024

fine for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants