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

Indent CSS functions #52

Merged
merged 2 commits into from
Feb 6, 2022
Merged

Indent CSS functions #52

merged 2 commits into from
Feb 6, 2022

Conversation

neruson
Copy link
Contributor

@neruson neruson commented Feb 5, 2022

I have some code in my template using CSS gradients that looks roughly like this:

<style>
    #gradient {
        background: linear-gradient(
            to top left,
            rgba(204,0,0,1),
            rgba(204,0,0,0)
        );
    }
</style>

However, djhtml removes the indentation in the function arguments:

<style>
    #gradient {
        background: linear-gradient(
        to top left,
        rgba(204,0,0,1),
        rgba(204,0,0,0)
        );
    }
</style>

This adds ( and ) as tokens in the CSS mode so CSS function arguments are indented one level.

Side note: I skipped creating an issue and went straight to a pull request, but I'm not sure if that's considered impolite. If so, I'm sorry, and I'm happy to go back and create one. Thank you for your hard work on this, I was really excited to discover this project!

@@ -199,7 +199,7 @@ Use your preferred system for setting up a virtualenv, docker environment,
or whatever else, then run the following:

```sh
python -m pip install -e .[dev]
python -m pip install -e '.[dev]'
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'm running zsh as my shell and without the quotes, this gave me this error: zsh: no matches found: .[dev]

if (
stack[-1].kind == token.kind
and stack[-1].is_hard == token.is_hard
):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These first two changes were automatically made by the black pre-commit hook.

Copy link
Member

Choose a reason for hiding this comment

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

Confirmed, pre-commit run --all on main produces the same changes. Not sure why, Black's version hasn't changed (although we should upgrade Black because they have released their first stable version!)

@neruson neruson marked this pull request as ready for review February 5, 2022 19:17
Copy link
Member

@JaapJoris JaapJoris left a comment

Choose a reason for hiding this comment

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

Thank you very much for this improvement. I am sure this will make many people happy, good job!

I just have one minor change request, after that I will merge your PR and release the next version crediting you as a new contributor.

Thanks again!

djhtml/modes.py Outdated
@@ -289,16 +294,18 @@ class DjCSS(DjTXT):
r"</style>",
r"{",
r"}",
r"\(",
r"\)",
Copy link
Member

Choose a reason for hiding this comment

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

Could you replace the previous 4 lines by just:

r"[\{\(\)\}]",

That would make it more similar to DjJS mode and accomplish the same thing.

if (
stack[-1].kind == token.kind
and stack[-1].is_hard == token.is_hard
):
Copy link
Member

Choose a reason for hiding this comment

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

Confirmed, pre-commit run --all on main produces the same changes. Not sure why, Black's version hasn't changed (although we should upgrade Black because they have released their first stable version!)

@neruson
Copy link
Contributor Author

neruson commented Feb 6, 2022

Thank you for the quick review! I like the idea of being consistent with the JS mode and I've made the change you suggested.

@JaapJoris JaapJoris merged commit 319a7b2 into rtts:main Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants