-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Make doc preview more prominent and remove headings #12295
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
base: main
Are you sure you want to change the base?
Conversation
I think the content is self-explanatory, and the huge header is not adding value. I think the primary action should be the biggest text. I'd like to see that become a button though.
Documentation build overviewFiles changedShow files (4) | 3 modified | 1 added | 0 deleted
|
We can probably use a badge here or a quick solution is to use |
I think we all have different taste here for how this comment should be and it seems we will be iterating quickly in the next weeks/months, so I'd suggest to mark this feature as beta and communicate clearly what to expect at this early stage. |
I think it's very verbose now, so we should definitely get three changes or similar in. |
@stsewd I'd still like to see something like this make it in. I find the current message has a lot of text that isn't really needed. |
### Files changed | ||
|
||
> Comparing with [{{ base_version.verbose_name }}]({{ base_version.get_absolute_url }}) ({{ base_version_build.commit }}...{{ current_version_build.commit }}) | ||
> 📚 [{{ project.name }}](https://{{ PRODUCTION_DOMAIN }}{% url "projects_detail" project.slug %}) | 🛠️ Build [#{{ current_version_build.pk }}](https://{{ PRODUCTION_DOMAIN }}{% url "builds_detail" project.slug current_version_build.pk %}) ({{ current_version_build.commit }}) | comparing [{{ base_version.verbose_name }}]({{ base_version.get_absolute_url }}) ({{ base_version_build.commit }}...{{ current_version_build.commit }}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
everything is lowercase here
> 📚 [{{ project.name }}](https://{{ PRODUCTION_DOMAIN }}{% url "projects_detail" project.slug %}) | 🛠️ Build [#{{ current_version_build.pk }}](https://{{ PRODUCTION_DOMAIN }}{% url "builds_detail" project.slug current_version_build.pk %}) ({{ current_version_build.commit }}) | comparing [{{ base_version.verbose_name }}]({{ base_version.get_absolute_url }}) ({{ base_version_build.commit }}...{{ current_version_build.commit }}) | |
> 📚 [{{ project.name }}](https://{{ PRODUCTION_DOMAIN }}{% url "projects_detail" project.slug %}) | 🛠️ build [#{{ current_version_build.pk }}](https://{{ PRODUCTION_DOMAIN }}{% url "builds_detail" project.slug current_version_build.pk %}) ({{ current_version_build.commit }}) | comparing [{{ base_version.verbose_name }}]({{ base_version.get_absolute_url }}) ({{ base_version_build.commit }}...{{ current_version_build.commit }}) |
|
||
> 📚 [{{ project.name }}](https://{{ PRODUCTION_DOMAIN }}{% url "projects_detail" project.slug %}) | 🛠️ build [#{{ current_version_build.pk }}](https://{{ PRODUCTION_DOMAIN }}{% url "builds_detail" project.slug current_version_build.pk %}) ({{ current_version_build.commit }}) | 🔍 [preview]({{ current_version.get_absolute_url }}) | ||
#### [View documentation preview]({{ current_version.get_absolute_url }}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really a fan of having this link in the header TBH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't like it either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel the heading is wrong. If you look at other providers, like Sentry, their comments have the same structure and include a heading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The heading is not bad, what I don't like is the heading being a link
The previous comment is my proposal for this message. |
There's still a ton of headings that take up space for no reason. We don't need a Your suggestion has |
Documentation build overviewComparing with latest (4617dfa): Show files (2) | 1 modified | 1 added | 0 deleted
|
I put another proposal based on the feedback. |
Documentation build overview Comparing with latest (4617dfa): Show files (2) | 1 modified | 1 added | 0 deleted
|
Something like that looks good, but we should definitely have viewing the preview as the primary action. I think that's the biggest thing people are going to want, and it should be the most visible. All the other items are more metadata, and less likely to be useful. |
I have mostly the same feedback there. I think we can keep the first header, the list of changes makes plenty of sense without a "Files changed" header though. But we can both retain the header and reduce the metadata sprawl by adding useful metadata into the heading instead. This is okay because users really shouldn't need to click into this metadata anyways. Users won't have a reason to click into the project from the comment and this comment will only reflect passing builds, so the build link is mostly useless to the user too -- users don't need to review passing build details. I'd still keep the preview link action out of the header entirely, the button is most clear. Using a button for the other metadata makes the function unclear though -- ie, does the "Build" button trigger a new build? The vertical list of metadata is mostly more wordy than the horizontal. We nixed this last review, I think the horizontal list still looks better. But I think we can get rid of this line entirely. Altogether, below:
This feels dense and none of the copy/data seems like filler content. Overview for project dev build #28730295Comparing c1cb4ee with latest (4617dfa): Show files changed (2 files total: 📝 1 modified | ➕ 1 added | ➖ 0 deleted)
|
I don't like the links on the header. They look too big and prominent to me, generating the opposite effect we want here (it shouldn't be the main action to do). Also, I propose to put these links outside the header. I prefer the bullets for this, but if we don't like them, I'm fine going back to the one line we had before. Based on these feedback, I have another proposal that's just a small header, 2 lines and the "Preview build" button. Looks pretty compact to me. Documentation build overview📚 Project dev | 🛠️ Build #28730295 (c1cb4ee) | 📁 Comparing c1cb4ee with latest (4617dfa) Show files changed (2 files total: 📝 1 modified | ➕ 1 added | ➖ 0 deleted)
|
I think the content is self-explanatory,
and the huge header is not adding value.
I think the primary action should be the biggest text.
I'd like to see that become a button,
but baby steps.