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

Integrate Tailwind framework #3999

Merged
merged 9 commits into from
Dec 23, 2022

Conversation

vitvakatu
Copy link
Contributor

@vitvakatu vitvakatu commented Dec 20, 2022

Pull Request Description

Task link

This PR moves the documentation visualization into a separate crate and sets up the Tailwind CSS framework for this new crate.

We would use Tailwind to style our HTML documentation.

2022-12-20.20-35-13.mp4

Important Notes

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@vitvakatu vitvakatu added the CI: No changelog needed Do not require a changelog entry for this PR. label Dec 20, 2022
@vitvakatu vitvakatu self-assigned this Dec 20, 2022
@vitvakatu vitvakatu marked this pull request as ready for review December 20, 2022 18:36
Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

The code is ok to merge, however I would like to discuss one thing: should the tailwind run be guarded by env variable? We run the build scripts which usually expects to have node installed anyway: therefore we could just assume we can safely run npx.

Perhaps the answer is still "yes", to cover some cases where we just want to run rust compilation natively, without using our build scripts and having no node in path. But the build script could set the RUN_TAILWIND env variable by default.

const TAILWIND_BINARY_NAME: &str = "tailwindcss";

fn main() {
println!("cargo:rerun-if-changed=src");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should also be re-run when environment changes. See https://doc.rust-lang.org/cargo/reference/build-scripts.html#cargorerun-if-env-changedname

Also add a comment explaining why we are attached to the entire src, not just CSS_INPUT_PATH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adeded

@vitvakatu
Copy link
Contributor Author

@farmaazon I don't want to modify our build script for now, as it can bring unnecessary complications. E.g., I'm unsure if the output CSS is always formatted and ordered in the same way.
After discussing the initial task design with @wdanilo, we decided to make the simplest possible solution.
From the task comments:

On the beginning, let's not integrate anything to build script, etc

I was considering not making any build script at all. Still, it turned out to be quite uncomfortable for the developer (You not only need to run the tailwind manually, but you also need to remember the exact parameters for it as well). So I decided to make a simple dumb script that would simplify life for developers.

I think the env variable cause should guard it; otherwise, you depend on the node, and again it brings complications If script generates a slightly different output on different machines.

Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

Request changes only because of commited ignored file - I just want to know what will happen with it before merge.

Comment on lines 1 to 7
/*
! tailwindcss v3.2.4 | MIT License | https://tailwindcss.com
*/

/*
1. Prevent padding and border from affecting element width. (https://github.com/mozdevs/cssremedy/issues/4)
2. Allow adding a border to an element by just adding a border-width. (https://github.com/tailwindcss/tailwindcss/pull/116)
Copy link
Member

Choose a reason for hiding this comment

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

Is this file meant to be gitignored? Because it is mentioned in gitignore but its committed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not gitignored. It is only mentioned in .prettierignore, as it is auto-generated. Keeping this file in VCS is a deliberate decision because, without it, the IDE (or, rather, the documentation panel) won't work correctly. It doesn't make much sense to regenerate it every build.

//! This script would run Tailwind CLI utility to generate a CSS stylesheet by scanning the
//! source code for class names and including needed CSS rules in the output file.
//!
//! This script would only run if the `RUN_TAILWIND` environment variable is defined.
Copy link
Member

Choose a reason for hiding this comment

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

Why? No information why such a design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote about it here: #3999 (comment)
Do I need to add this info to the build script docs?

@vitvakatu
Copy link
Contributor Author

After discussion with @wdanilo we decided to remove generated stylesheet from the VCS and instead regenerate it on each build.

@vitvakatu
Copy link
Contributor Author

As per discussion with @mwu-tow , the resulting stylesheet is now stored in the OUT_DIR to avoid issues on CI.

@vitvakatu vitvakatu added the CI: Ready to merge This PR is eligible for automatic merge label Dec 23, 2022
@mergify mergify bot merged commit 37af06b into develop Dec 23, 2022
@mergify mergify bot deleted the wip/vitvakatu/tailwind-integration-183992025 branch December 23, 2022 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants