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

Implement UI for automation rules #5996

Merged
merged 51 commits into from Nov 12, 2019

Conversation

@stsewd
Copy link
Member

stsewd commented Jul 25, 2019

This is on top of #5995 (that PR should be merged first)

Still wip, and I need to write tests, and a little of js :)

And I need to add actions to increase/decrease the priority

@stsewd

This comment has been minimized.

Copy link
Member Author

stsewd commented Aug 13, 2019

Some screenshots.

Still missing the actions to move rules (up and down)

Screenshot_2019-08-13 Automation Rule Read the Docs(7)

Screenshot_2019-08-13 Automation Rules Read the Docs(1)
Screenshot_2019-08-13 Automation Rule Read the Docs(4)
Screenshot_2019-08-13 Automation Rule Read the Docs(5)
Screenshot_2019-08-13 Automation Rules Read the Docs(2)

@stsewd

This comment has been minimized.

Copy link
Member Author

stsewd commented Aug 13, 2019

And not sure how/where to put the delete button, couldn't find any other place where we have something like this. We have to delete button in the list or a link to a two steps confirmation.

@humitos

This comment has been minimized.

Copy link
Member

humitos commented Aug 13, 2019

We have to delete button in the list or a link to a two steps confirmation.

I think the pattern of a link with the confirmation step is the one that applies in this case.

Copy link
Member

humitos left a comment

Code looks good to me! I left some comments about style and naming.

I think we need to improve the UI a little more to make it explicit to the user what is all this about. As a user, going to that page and taking a look at the name of the fields at rule creation is not easy to realize what the feature is for.

Also, I like the idea of having a way to write a custom regex, but the UI may say something like "Advanced" or similar warning the user that this is complicated. Besides, in the help text of that field, we may want to link to a regex documentation or the Python Regex documentation itself.

readthedocs/builds/managers.py Outdated Show resolved Hide resolved
readthedocs/builds/forms.py Outdated Show resolved Hide resolved
readthedocs/builds/forms.py Outdated Show resolved Hide resolved
readthedocs/builds/forms.py Outdated Show resolved Hide resolved
readthedocs/builds/forms.py Show resolved Hide resolved
def clean_description(self):
description = self.cleaned_data['description']
if not description:
description = self.instance.get_description()

This comment has been minimized.

Copy link
@humitos

humitos Aug 13, 2019

Member

I think it's better to leave it blank if the user does not enter any description and make this check when rendering. This way we are not duplicating the same value over and over in the db and also if we change it later with a better message, it will change for all the users.

This comment has been minimized.

Copy link
@stsewd

stsewd Aug 13, 2019

Author Member

But then the user would see the description in list page and not on the form, I think that would be confusing

This comment has been minimized.

Copy link
@humitos

humitos Aug 14, 2019

Member

I'm fine with that.

Although, if you want to show it in the form as well, you can add it in the Form.__init__ method.

This comment has been minimized.

Copy link
@stsewd

stsewd Aug 14, 2019

Author Member

We can't do that bc we don't know what option is going to be selected yet

This comment has been minimized.

Copy link
@humitos

humitos Aug 14, 2019

Member

The thing is that you want to populate that field with the default text only when editing the rule. When creating the rule, the field should be empty so the user can write anything they want.

This comment has been minimized.

Copy link
@stsewd

stsewd Aug 14, 2019

Author Member

Is empty by default, if the form is saved without a description, the rule is saved with the default description, otherwise use what the user wrote

This comment has been minimized.

Copy link
@humitos

humitos Aug 14, 2019

Member

Right. Why do we want to save the same value over and over again in the database if it's the default when it's None? That's my point.

readthedocs/builds/managers.py Outdated Show resolved Hide resolved
readthedocs/builds/managers.py Outdated Show resolved Hide resolved
readthedocs/builds/models.py Outdated Show resolved Hide resolved
{% block project_edit_content_header %}{% trans "Automation Rule" %}{% endblock %}

{% block project_edit_content %}
<p>Run specific actions when a new version is added.</p>

This comment has been minimized.

Copy link
@humitos

humitos Aug 13, 2019

Member

This paragraph should be expanded a little more and link to the the docs for more information probably.

This comment has been minimized.

Copy link
@stsewd

stsewd Aug 13, 2019

Author Member

I don't think adding more words here is the best, a link to the docs should be better. #6059.

@stsewd

This comment has been minimized.

Copy link
Member Author

stsewd commented Aug 13, 2019

@humitos this is on top of #5995, so some of your comments don't apply to this PR, you should review the other one first

@stsewd

This comment has been minimized.

Copy link
Member Author

stsewd commented Aug 13, 2019

This is with the up/down actions. This needs #5998

Screenshot_2019-08-13 Automation Rules Read the Docs(3)

@humitos

This comment has been minimized.

Copy link
Member

humitos commented Aug 13, 2019

so some of your comments don't apply to this PR, you should review the other one first

Feel free to use my comments from here to apply them in the other PR 😉

@stsewd

This comment has been minimized.

Copy link
Member Author

stsewd commented Aug 13, 2019

I think we are moving away from two steps confirmation to delete #1784

This is from webhooks

Screenshot_2019-08-13 Integration Read the Docs

@stsewd

This comment has been minimized.

Copy link
Member Author

stsewd commented Nov 6, 2019

I've improved the UI, removing help text that wasn't needed and now the help text for the predefined rules change when the option changes.

To allow us to modify the regex of the predefined options, I've created a new field. That way all users that chosen a predefined option get the update.

Screenshot_2019-11-06 Automation Rule Read the Docs(4)
Screenshot_2019-11-06 Automation Rule Read the Docs(3)
Screenshot_2019-11-06 Automation Rule Read the Docs(2)
Screenshot_2019-11-06 Automation Rule Read the Docs(1)
Screenshot_2019-11-06 Automation Rule Read the Docs
Screenshot_2019-11-06 Automation Rules Read the Docs

@stsewd stsewd requested a review from readthedocs/core Nov 6, 2019
@stsewd

This comment has been minimized.

Copy link
Member Author

stsewd commented Nov 6, 2019

This needs #6163 before going live. Also, I was wondering if we want to put this under a feature flag or just release it?

Copy link
Contributor

agjohnson left a comment

Changes look good! I haven't QAed, but i like the defaults for regex, that's a helpful ux touch.

I did note some changes required to move JS out of inline JS -- there are a few reasons we avoid this. I did also note some a11y fixes as well. We don't have a focus on a11y best practices for the dashboard, and it's not really possible to say what percentage of users use assistive technologies, have but it doesn't take much work do something for better accessibility. Those aren't as important, but the changes are pretty minor anyways.

The blocking part of this feedback is the JS, it should be moved to a standalone file.

set_help_text(this.value);
});
});
</script>

This comment has been minimized.

Copy link
@agjohnson

agjohnson Nov 8, 2019

Contributor

This should go in a standalone JS file. We don't do inline javascript as it is not as maintainable and is not testable.

This comment has been minimized.

Copy link
@agjohnson

agjohnson Nov 8, 2019

Contributor

Also, this is the type of place to use knockout if you feel like it. This JS replicates a lot of what knockout was made for. If you haven't touched knockout yet, I can help with this in a separate pr.

This comment has been minimized.

Copy link
@stsewd

stsewd Nov 11, 2019

Author Member

I was trying to use knockout but looks like we need to explicitly put data-bind in the form, for that we'll need to manually render the form I guess. Not sure if we want that. But I was able to move it to a new file.

This comment has been minimized.

Copy link
@agjohnson

agjohnson Nov 12, 2019

Contributor

👍 a separate file is the most important. We can address the use of knockout on the form separately.

It's possible to use data-bind without a manual render with something ike this on the form:

foo = forms.CharField(
    required=True,
    widget=forms.HiddenInput(
        attrs={
            'data-bind': 'valueInit: foo',
        },
    ),
)

There are also helper wrappers for displaying knockout forms that need custom rendering:

<div
class="subscription-card"
data-bind="visible: show_card_form"
style="display: none;">
{% for groupfield in field.fields %}
{% include 'core/ko_form_field.html' with field=groupfield %}
{% endfor %}
</div>

readthedocs/builds/forms.py Outdated Show resolved Hide resolved
stsewd and others added 5 commits Nov 11, 2019
Co-Authored-By: Anthony <aj@ohess.org>
@agjohnson

This comment has been minimized.

Copy link
Contributor

agjohnson commented Nov 12, 2019

Changes look good! I'll set this as a major item for next deploy. Maybe lets create an issue to refactor the JS to use knockout, perhaps a comment in the JS file to not re-use the pattern as well, so we don't base new work off it.

@stsewd

This comment has been minimized.

Copy link
Member Author

stsewd commented Nov 12, 2019

Opened #6373

@stsewd stsewd merged commit f46405f into readthedocs:master Nov 12, 2019
2 checks passed
2 checks passed
continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stsewd stsewd deleted the stsewd:implement-ui-for-automation-rules branch Nov 12, 2019
@ericholscher

This comment has been minimized.

Copy link
Member

ericholscher commented Nov 13, 2019

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.