-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Regroup settings #5459
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
Regroup settings #5459
Conversation
This is an initial proposal for readthedocs#5413
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'm +1 on moving documentation_type
to advanced as I do think that's a configuration option that few people need to change. It also can be configured in the YAML file.
I think the order is pretty good (I made 1 comment there).
I'm also +1 on crispy forms overall.
'default_version', | ||
'default_branch', | ||
'privacy_level', | ||
'analytics_code', |
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.
This comment is outside the scope of this PR, but we could put this setting into the config file on a per-version basis. It might be nice to get that information versioned.
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.
Any per-version config, is excellent for the YAML. I agree with David to implement this in another PR.
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.
Don't people use just one analytics code per site? And also, isn't a way of identify each page from analytics?
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 opened #5478
It can be in a separate PR and it would be a lot easier if we had crispy forms but I think we do need to have a note in here to the effect of "These settings can be configured in your project's configuration file and settings in the configuration file override the settings listed here". |
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 like these changes.
👍 on crispy forms. Although, it's not something that needs to be done in this PR. Actually, I think it would be good to make a bigger refactor when introducing crispy forms and upgrade other forms also.
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.
Better than what we have now. 👍
This is an initial proposal for #5413
This only affects the order of the current settings, we can create real groups modifying the template, using two forms or using https://django-crispy-forms.readthedocs.io/en/latest/layouts.html
For now, I would like to get reviewed the order/group of these options, and if someone has an opinion about the best way of having separate groups using the adobe suggestions.