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

Regroup settings #5459

Merged
merged 3 commits into from
Mar 18, 2019
Merged

Regroup settings #5459

merged 3 commits into from
Mar 18, 2019

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Mar 14, 2019

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.

This is an initial proposal for readthedocs#5413
@stsewd stsewd requested review from davidfischer and a team March 14, 2019 16:32
@stsewd
Copy link
Member Author

stsewd commented Mar 14, 2019

This is how it looks like

Screenshot_2019-03-14 Edit Advanced Project Settings Read the Docs

Copy link
Contributor

@davidfischer davidfischer left a 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',
Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #5478

readthedocs/projects/forms.py Outdated Show resolved Hide resolved
@davidfischer
Copy link
Contributor

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

Copy link
Member

@humitos humitos left a 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.

Copy link
Member

@ericholscher ericholscher left a 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. 👍

@stsewd stsewd merged commit 4291265 into readthedocs:master Mar 18, 2019
@stsewd stsewd deleted the regroup-settings branch March 18, 2019 15:50
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