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

Import project: suggest a config file (yaml) #166

Merged
merged 2 commits into from
Jul 11, 2023

Conversation

humitos
Copy link
Member

@humitos humitos commented May 30, 2023

I tried to add the intermediate page where we suggest a common Sphinx YAML file for the user to copy and paste.

I found the pattern a little hard to follow. The base template didn't have all the blocks I needed, so I added some more but I don't feel comfortable with them.

I also had to create a block to overwrite the CSS classes of the main div, which sounds weird to me as well.

Besides, I'm not sure how to use the CSS classes to achieve what I need: syntax highlighting, smaller text, etc.

Related readthedocs/readthedocs.org#10356
Closes #165

@humitos
Copy link
Member Author

humitos commented May 30, 2023

I'd need lot of feedback on this PR and mainly guidance about how it's expected for us to work on this project. I have no idea about how to use the CSS classes to achieve my goals here. This is what I have for now:

Screenshot_2023-05-30_12-18-33

@ericholscher ericholscher requested a review from agjohnson June 29, 2023 15:40
@agjohnson
Copy link
Contributor

This one is still on my list to get to, I'll need to pull it down and experiment. I'm touching the project creation templates still, so don't want to point in the wrong direction here yet. But, I will have some guidance on this. I looked at the element structure, and don't know immediately what I'd suggest to improve this, but I can pull this down next to see.

I would suggest probably reusing the structure for the code block from the build detail template, including the pygement wrapper classes, for the code block here. You also shouldn't need to adjust the base template. But, I'm guessing it's probably just a matter of finding the right order of elements for content alignment/etc.

@agjohnson
Copy link
Contributor

agjohnson commented Jul 6, 2023

I'll add some notes here for some reference, but in pulling this down to help with some guidance, I already made a few changes that this PR could use. But still, I'll note what I did here and probably move to merge this early so my work doesn't have to stay in conflict for any longer.

The main thing that this PR needed is some element structure to help constrain the vertical height. The form is quite long with a big example YAML in the middle and it's not super clear that it's even a form until you scroll to the bottom. I could see a modal being better here overall, but I used a scrolling segement, which is a native FUI element that allows for scrolling inside the segment.

I also reused the element structure from some of our existing code blocks -- build detail page pseudo terminal, the integration HTTP request/response, etc. This provides a clear designation between the code example and the rest of the page.

I also dialed back on the block elements that were changed/removed, and kept the help topics list on the config form page.

image

agjohnson added a commit that referenced this pull request Jul 7, 2023
I pulled down #166 to provide a bit of guidance, but ended up with fixes
to the PR either way. I added a few features on top, to replace plain
text with visual elements and interactive components. These aren't fully
working yet (the example selector namely, see #184).
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

I pulled this one down to experiment and give a bit of feedback, but ended up coming up with a few fixes in the process. I put these up for review in #185.

I only wanted to poked around on the form column widths, but ended up experimenting a bit more here and in the process solved a few points I wanted to put in review.

I opened up that PR so we could review the differences, but most of the commentary is in this review here.

readthedocsext/theme/templates/projects/import_base.html Outdated Show resolved Hide resolved
<p class="info">
Here you have an example for a common Sphinx project:

<pre class="ui padded">
Copy link
Contributor

Choose a reason for hiding this comment

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

These classes are swapped, usually pre is inside code -- code is the block type and pre is for whitespace control.

Additionally, I dropped the ui padded here and wrapped everything with a few more elements. The main element was ui inverted scrolling segment -- this provides an inverted color background (it's vaguely terminal-like), and scrolls with the user. This constrains the vertical space the code block can take up, and hopefully allows the form buttons to show above the fold for users. The inverted color allows provides some visual clue that the code block might terminate at some point.

I also added another element in my example, but it's extra -- it's a top attached label with the file name and a copy button.

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 dropped the ui padded here and wrapped everything with a few more elements

How do you know what are the meaning of these classes? This is probably my main struggle with this type of work. I just copied and pasted ui padded from somewhere else because I don't know how to use them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have talked about writing up doc pages for an introduction into authoring new UI, this would be good to point out.

All of the elements and their variations are documented in the documentation, but you need to know the element in order to look up the documentation. In this case its a segment element and the docs for padded are here: https://fomantic-ui.com/elements/segment.html#padded

I always use <div class="ui [varations...] [element_name]"> to keep this more clear what element is being used.

I used:

And you can see the usage in all of the examples by clicking on the show source buttons:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened up #187 to cover some topics like these, I might want to do this sometime this week after we get a deploy out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! This comments explains a lot of things 👍🏼

<div class="ui very padded centered stackable grid">
<div class="ui ten wide tablet six wide computer column">
{% block project_add_content %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this block change isn't required. I wanted to keep the help topics column however.

readthedocsext/theme/templates/projects/import_base.html Outdated Show resolved Hide resolved
readthedocsext/theme/templates/projects/import_base.html Outdated Show resolved Hide resolved
readthedocsext/theme/templates/projects/import_config.html Outdated Show resolved Hide resolved

{% block project_add_css_classes %}ui ten wide tablet wide computer column{% endblock project_add_css_classes %}
{% block project_add_content_main %}
<p class="info">
Copy link
Contributor

Choose a reason for hiding this comment

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

The info class is not a FUI element, so can be removed. For a block like this, I'd use something to provide the user with more visual clues, such as:

Suggested change
<p class="info">
<p class="ui small info message">

readthedocsext/theme/templates/projects/import_config.html Outdated Show resolved Hide resolved
agjohnson added a commit that referenced this pull request Jul 7, 2023
- 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
agjohnson added a commit that referenced this pull request Jul 11, 2023
* Tweak project create layout and elements

- 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

* Unify project creation columns and layout

Avoid switching layouts multiple times, always use the same two columns.
This is more predictable for the user and for us.

* Typo fixes

Co-authored-by: Manuel Kaufmann <humitos@gmail.com>

---------

Co-authored-by: Manuel Kaufmann <humitos@gmail.com>
agjohnson added a commit that referenced this pull request Jul 11, 2023
* Some additions to the project import configuration page

I pulled down #166 to provide a bit of guidance, but ended up with fixes
to the PR either way. I added a few features on top, to replace plain
text with visual elements and interactive components. These aren't fully
working yet (the example selector namely, see #184).

* Update JS comment to be accurate
humitos and others added 2 commits July 10, 2023 21:19
I tried to add the intermediate page where we suggest a common Sphinx YAML file
for the user to copy and paste.

I found the pattern a little hard to follow. The base template didn't have all
the blocks I needed, so I added some more but I don't feel comfortable with
them.

I also had to create a block to overwrite the CSS classes of the main div, which
sounds weird to me as well.

Besides, I'm not sure how to use the CSS classes to achieve what I need: syntax
highlighting, smaller text, etc.
* Some additions to the project import configuration page

I pulled down #166 to provide a bit of guidance, but ended up with fixes
to the PR either way. I added a few features on top, to replace plain
text with visual elements and interactive components. These aren't fully
working yet (the example selector namely, see #184).

* Update JS comment to be accurate
@agjohnson agjohnson force-pushed the humitos/import-wizard-config branch from 3d33c89 to 16b38df Compare July 11, 2023 04:30
@agjohnson agjohnson marked this pull request as ready for review July 11, 2023 04:31
@agjohnson agjohnson requested a review from a team as a code owner July 11, 2023 04:31
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

I've rebased everything here, hopefully without breaking anything 🤞

@agjohnson agjohnson merged commit 6b328c3 into main Jul 11, 2023
@stsewd stsewd deleted the humitos/import-wizard-config branch July 24, 2023 17:10
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.

Import wizard: suggest a .readthedocs.yaml file
2 participants