Skip to content

Tune project creation layout and UI #186

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

Merged
merged 3 commits into from
Jul 11, 2023
Merged

Conversation

agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Jul 7, 2023

Screencast.from.2023-07-07.12-41-09.webm

Closes #150

agjohnson added 2 commits July 7, 2023 10:42
- Make the search more obvious it is a search
- Drop the mention of syncing repositories down to the bottom of the
  page. This should be a last resort in most cases, but still needed UX
- Tune the copy to point the user to "search for a project", instead of
  instructions on not finding a repository
- Tune some of the dynamic elements used in the search results
- Some changes related to #166 and #185
Avoid switching layouts multiple times, always use the same two columns.
This is more predictable for the user and for us.
@agjohnson agjohnson requested a review from a team as a code owner July 7, 2023 19:40
{% endblock wizard_form %}

{% csrf_token %}
{{ wizard.management_form }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the hidden input fields, which are dropped down so that the .ui.fieldset.segment that wraps all of the fields is the true first child element of the <form>. This introduces less margin/padding at the top of the element.

@@ -8,3 +7,4 @@
{% include "semantic-ui/field.html" %}
{% endfor %}
</div>
{{ form.media }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was another attempt to put the form hidden inputs below the fieldset definition. I don't see any breaking change from this but it should be able to be safely reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much of the structure here was moved to the base template and reused there instead. This provides a singular column layout for all the project creation pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The column layout from import_form was moved here so all child templates could share this. I recommend viewing all of this with whitespace ignored.

@agjohnson agjohnson requested a review from humitos July 7, 2023 19:50
@agjohnson
Copy link
Contributor Author

This is a bit heavy to review, but I opened this to help provide some insight on the template changes that affect #166 and friends. @humitos I'm happy to discuss any of the changes here, this review is probably more for sharing knowledge than anything.

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.

The video looks great. I tried to review this work but I don't feel capable of doing a proper review of the code itself. I still don't really know how to provide code feedback here since I don't really understand how SUI works and what is the meaning of all these CSS classes.

I suppose we will need something like https://github.com/readthedocs/meta/discussions/114 but for ext-theme (SUI, Knockout.js, and anything required to understand this project).

Comment on lines +30 to +38
<script type="application/json" data-bind="jsonInit: config">
{
"urls": {
"api_sync_remote_repositories": "{% url 'api_sync_remote_repositories' %}",
"remoterepository_list": "{% url 'remoterepository-list' %}"
},
"csrf_token": "{{ view_csrf_token }}"
}
</script>

{# Sidebar #}
<div class="ui sixteen wide tablet five wide computer four wide large screen column">
<div class="ui fluid secondary pointing vertical menu" data-bind="css: {vertical: device.computer()}">
<a class="item active" data-tab="automatic">
<i class="fa-duotone fa-magic icon"></i>
{% trans "Configure automatically" %}
</a>
<a class="item" data-tab="manual">
<i class="fa-duotone fa-hand-sparkles icon"></i>
{% trans "Configure manually" %}
</a>
</div>
</div>
{# End sidebar #}
</script>
Copy link
Member

Choose a reason for hiding this comment

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

How is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The jsonInit binding is used in the templates to bring backend data into the Knockout view where an API response is not really possible. In many cases, an API response will have the URLs that we would use in the Knockout views, but for cases where this isn't true the jsonInit binding can be used.

The API reference docs on jsonInit have some more background and examples.

</div>
{% endblock project_add_sidebar_content %}

{% block project_add_data_bind %}data-bind="using: ProjectCreateView()"{% endblock %}
Copy link
Member

Choose a reason for hiding this comment

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

Where does ProjectCreateView lives? How/When/Why is it called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ProjectCreateView is a Knockout view, documented some here.

All Knockout views are surfaced through the main view, ApplicationView. This registers all of the views as globals to Knockout.

These views are all used by defining a Knockout using data binding, which is a native KO data binding. This changes the current Knockout context to the instance of the view -- ProjectCreateView in this case.

It's done this way to avoid maintaining some sort of URL routing in the frontend code. The backend already did the URL routing, so templates instead reference the Knockout views they use in a very explicit way.

Comment on lines +107 to +108
<div class="ui basic horizontally fitted segment" data-bind="visible: !is_selected()">
<div class="ui top attached placeholder segment">
Copy link
Member

Choose a reason for hiding this comment

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

What's the documentation I have to read to understand all these CSS classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You should look at the FomanticUI docs, for example the segment element docs cover all of the variations for segment elements.

Co-authored-by: Manuel Kaufmann <[email protected]>
@agjohnson
Copy link
Contributor Author

I suppose we will need something like https://github.com/readthedocs/meta/discussions/114 but for ext-theme (SUI, Knockout.js, and anything required to understand this project).

Yeah, this is a piece I've wanted for some time too. I added #187 earlier to start accumulating more guide content in the ext-theme docs.

@agjohnson agjohnson merged commit 195ed9f into main Jul 11, 2023
@stsewd stsewd deleted the agj/project-create-updates branch July 24, 2023 17:08
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.

Project: create view UI updates
2 participants