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

update blank template #3219

Merged
merged 5 commits into from
May 8, 2024
Merged

update blank template #3219

merged 5 commits into from
May 8, 2024

Conversation

Lendemor
Copy link
Collaborator

@Lendemor Lendemor commented May 3, 2024

No description provided.

Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

Is the content supposed to be left aligned now? i actually think it looks kinda cool that way.

The other issue is in chrome there is always a scroll bar now, regardless of the window height (on my screen anyway). min_height="88vh" or something a bit smaller might help there.

@Lendemor
Copy link
Collaborator Author

Lendemor commented May 3, 2024

Is the content supposed to be left aligned now? i actually think it looks kinda cool that way.

The other issue is in chrome there is always a scroll bar now, regardless of the window height (on my screen anyway). min_height="88vh" or something a bit smaller might help there.

That's the intention yeah.

I can tweak the value for min_height yeah, didn't happens on my screen.

Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

while we're touching this, can we rearrange the imports too?

I'd rather see

import reflex as rx

from rxconfig import config

filename = f"{config.app_name}/{config.app_name}.py"
  1. this is the preferred pep8-style import order, local modules should be listed after installed modules
  2. this makes it easier to delete without removing the reflex import

i also think we should just inline the docs_url, instead of defining it at the top of the module

@Lendemor
Copy link
Collaborator Author

Lendemor commented May 6, 2024

while we're touching this, can we rearrange the imports too?

I'd rather see

import reflex as rx

from rxconfig import config

filename = f"{config.app_name}/{config.app_name}.py"
  1. this is the preferred pep8-style import order, local modules should be listed after installed modules
  2. this makes it easier to delete without removing the reflex import

i also think we should just inline the docs_url, instead of defining it at the top of the module

I tried, but we literally can't, because reflex is considered as a local, ruff change the import back to how they are at the moment.

@benedikt-bartscher
Copy link
Contributor

@Lendemor maybe you can add a ruff config overwrite for the template dir: https://docs.astral.sh/ruff/configuration/#config-file-discovery

maybe smth like this (untested):

[tool.ruff]
# Extend the `pyproject.toml` file in the parent directory...
extend = "../pyproject.toml"

[tool.ruff.isort]
known-first-party = []

@Lendemor
Copy link
Collaborator Author

Lendemor commented May 7, 2024

@Lendemor maybe you can add a ruff config overwrite for the template dir: https://docs.astral.sh/ruff/configuration/#config-file-discovery

maybe smth like this (untested):

[tool.ruff]
# Extend the `pyproject.toml` file in the parent directory...
extend = "../pyproject.toml"

[tool.ruff.isort]
known-first-party = []

Thanks for the suggestion, we went with excluding the rule I001 for only blank.py, should be good enough since all the other templates live in their own repo and won't have that problem.

@Lendemor Lendemor requested a review from masenf May 7, 2024 10:57
@masenf masenf merged commit bdcbe00 into main May 8, 2024
46 checks passed
@picklelo picklelo deleted the lendemor/update_blank_template branch August 21, 2024 20:43
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.

4 participants