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 advanced settings #5489

Merged
merged 8 commits into from Mar 22, 2019

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Mar 19, 2019

This is a continuation of #5459

Closes #5413

It looks like this

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

I'm not sure about the texts, so any help is welcome.

readthedocs/projects/forms.py Outdated Show resolved Hide resolved
@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 19, 2019

I haven't tested it with the corporate site, first I want to know if we are ok with this :)

@@ -0,0 +1,9 @@
{% load i18n %}
<p class="empty">
Copy link
Member Author

@stsewd stsewd Mar 19, 2019

Used the same patter that we have in settings https://readthedocs.org/dashboard/docs/edit/

@stsewd stsewd requested review from davidfischer and Mar 19, 2019
@humitos
Copy link
Member

@humitos humitos commented Mar 19, 2019

I think we could adapt the note in the second group to say something like: "These settings can be defined in the YAML file. We strongly recommend that." or something like that inviting people to use the config file instead of the web.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Mar 19, 2019

I think we could adapt the note in the second group to say something like: "These settings can be defined in the YAML file. We strongly recommend that." or something like that inviting people to use the config file instead of the web.

Agreed with @humitos -- the goal here is to push people to use YAML. We should also likely have logic here that shows if they already have a YAML config in their builds, and then puts a big warning that these settings won't do anything, if they do.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 19, 2019

We should also likely have logic here that shows if they already have a YAML config in their builds, and then puts a big warning that these settings won't do anything, if they do.

But we'll be lying, these settings will still affect to the versions without a config file, we can't determinate exactly that sadly.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 19, 2019

But we can show that in the build page :), I'm creating another issue to track that

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Mar 19, 2019

But we'll be lying, these settings will still affect to the versions without a config file, we can't determinate exactly that sadly.

Sure, but we can detect if any versions use a config, and make the warning more pronounced. We could even presumably query all active versions and say "these settings will apply to 2 out of 6 versions)" if it isn't too heavy on the DB.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Mar 20, 2019

I think we should avoid making our UX for this view more complicated and instead frame these as "Default options" -- meaning, there is a path to overriding the options. Adding display to the view that adds information on which versions use which configuration option feels like we're putting emphasis on the wrong UI.

The thing we can do here is completely hide the options for new projects, instead pointing them to the config file for all their versions. But i think we're addressing that separately.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 20, 2019

The thing we can do here is completely hide the options for new projects, instead pointing them to the config file for all their versions. But i think we're addressing that separately.

I put some thoughts about that in #5413

I was thinking in blocking the UI for new projects, but we can't do that, because that would be a problem for people moving to read the docs (they can't modify their docs). A long solution can be allowing to have a global configuration (codecov has this feature), and that can be merged with the configuration of each version (probably I can create another issue to discuss this more if we agree).

Copy link
Contributor

@davidfischer davidfischer left a comment

I added a few minor bits of feedback but I'm pretty happy with this change overall. Great job!

The thing we can do here is completely hide the options for new projects, instead pointing them to the config file for all their versions. But i think we're addressing that separately.

I'm 👍 on doing this generally but I do agree that's a larger change involving a couple pieces and may not be best suited for here.

@@ -0,0 +1,10 @@
{% load i18n %}
<p class="empty">
{% blocktrans with config_file_link="https://docs.readthedocs.io/page/config-file" %}
Copy link
Contributor

@davidfischer davidfischer Mar 21, 2019

I'd link directly to the v2 config file docs: https://docs.readthedocs.io/page/config-file/v2.html

Copy link
Contributor

@davidfischer davidfischer Mar 21, 2019

Also, you might consider adding trimmed to the blocktrans tag.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 22, 2019

Looks like we need to fix something in the styles of the corporate site :)

Screenshot_2019-03-21 Edit Advanced Project Settings - Read the Docs for Business

  • No borders (maybe this is part of the style?)
  • Submit button
  • Help/dialog text looks 💯

Copy link
Member

@ericholscher ericholscher left a comment

This looks good. Let's ship it 👍

readthedocs/projects/forms.py Outdated Show resolved Hide resolved
Co-Authored-By: stsewd <santos_g@outlook.com>
@stsewd stsewd merged commit 44951fb into readthedocs:master Mar 22, 2019
1 check passed
@stsewd stsewd deleted the use-crispy-forms-advanced-settings branch Mar 22, 2019
@@ -13,7 +14,7 @@

{% block project_edit_content %}
<form method="post" action=".">{% csrf_token %}
{{ form.as_p }}
{% crispy form %}
<p>
<input style="display: inline;" type="submit" value="{% trans "Save" %}">
Copy link
Member

@dojutsu-user dojutsu-user Mar 27, 2019

@stsewd
{% crispy form %} renders

<form>
  <!-- csrf token -->
  <!-- content -->
</form>

And we have wrapped this in outer <form> tag and it is finally rendering something like this.
Screenshot from 2019-03-27 21-50-05

and so the Save button, which is rendered outside <form> tag, is not working anymore. We need to fix this before merging this in rel branch

docs: https://django-crispy-forms.readthedocs.io/en/latest/crispy_tag_forms.html#fundamentals

Copy link
Member Author

@stsewd stsewd Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants