Remove PR build notification and add info to Visual Diff #548
Conversation
This removes the PR build notification, and includes some information in the Visual Diff UI with similar information.
|
This is like a baby command palette :D |
|
It's odd to put unrelated links in this dropdown. The dropdown by default is going to have a selected value of The more standard place to put this is under a kebab dropdown menu, or add the information circle icon with some sort of popup/etc. I think what we talked about in the meeting was clickable element in the UI instead of repurposing the dropdown. I still like the default text addition to the dropdown, this adds enough context to drop the notification |
| @@ -197,10 +198,28 @@ export class FileTreeDiffElement extends LitElement { | |||
| ${this.renderDocDiff()} | |||
| <select @change=${this.handleFileChange}> | |||
| <option value="" ?selected=${!hasCurrentFile} disabled> | |||
| Files changed | |||
| Files changed in #${this.config.versions.current.slug} | |||
There was a problem hiding this comment.
This needs to be conditional based on what the build is it seems. If it's a named branch, we should not use a # prefix, and if this is a pull request we want more descriptive PR #1234 instead here. This makes it immediately recognizable that this is a pull request build, without expanding this menu.
| "notification", | ||
| )}" | ||
| > | ||
| Go to build log |
There was a problem hiding this comment.
The verb seems unneeded on these. These would also feel more natural under a dedicated menu, which would remove the need to explain that these are links that go to another page.
| Go to build log | |
| Build log |
|
I agree with Anthony here. I would
I understand that's what we agreed in our meeting. |
|
I opened a PR with these changes in #551 |
This removes the PR build notification,
and includes some information in the Visual Diff UI with similar information.
I tested with a real version, but these would be PR #'s in normal usage:
Refs https://github.com/readthedocs/meta/issues/176