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

UX: improve UI of software update page & display more info. #214

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,62 +1,72 @@
<tr>
<tr class="repo {{if @repo.hasNewVersion 'new-version'}}">
<td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please convert this component to a .gjs component while you are at it?

{{#if this.officialRepoBadge}}
{{d-icon
this.officialRepoBadge
translatedTitle=this.officialRepoBadgeTitle
class="check-circle"
}}
<div class="repo__name">
{{#if @repo.linkUrl}}
<a href={{@repo.linkUrl}} rel="noopener noreferrer" target="_blank">
{{@repo.nameTitleized}}
</a>
{{else}}
{{@repo.nameTitleized}}
{{/if}}
</div>
{{#if @repo.author}}
<div class="repo__author">
{{@repo.author}}
</div>
{{/if}}
{{#if @repo.plugin}}
<div class="repo__about">
{{@repo.plugin.about}}
{{#if @repo.linkUrl}}
<a href={{@repo.linkUrl}} rel="noopener noreferrer" target="_blank">
{{i18n "admin.plugins.learn_more"}}
</a>
{{/if}}
</div>
{{/if}}
{{#if @repo.hasNewVersion}}
<div class="repo__new-version">
{{i18n "admin.docker.new_version_available"}}
</div>
{{/if}}
</td>

<td>
<a href={{@repo.url}}>{{@repo.name}}</a>
<span class="current commit-hash" title={{@repo.version}}>
{{@repo.prettyVersion}}
</span>
</td>

<td>{{format-date @repo.latest.date leaveAgo="true"}}</td>
<td>
<ul class="repo__latest-version">
<li>
<span class="new commit-hash" title={{@repo.latestVersion}}>
{{@repo.prettyLatestVersion}}
</span>
</li>
<li class="new-commits">
{{new-commits
@repo.latest.commits_behind
@repo.version
@repo.latest.version
@repo.url
}}
</li>
</ul>
</td>
<td class="repo__status">
{{#if @repo.checkingStatus}}
{{i18n "admin.docker.checking"}}
{{else if @repo.upToDate}}
{{i18n "admin.docker.up_to_date"}}
{{else}}
<div class="new-version">
<h4>{{i18n "admin.docker.new_version_available"}}</h4>

<ul>
<li>
{{i18n "admin.docker.latest_version"}}
<span class="new commit-hash" title={{@repo.latestVersion}}>
{{@repo.prettyLatestVersion}}
</span>
</li>
<li>
{{i18n "admin.docker.last_updated"}}
{{#if @repo.latest.date}}
{{format-date @repo.latest.date}}
{{else}}
&mdash;
{{/if}}
</li>
<li class="new-commits">
{{new-commits
@repo.latest.commits_behind
@repo.version
@repo.latest.version
@repo.url
}}
</li>
</ul>

<DButton
@action={{this.upgrade}}
@disabled={{this.upgradeDisabled}}
@translatedLabel={{this.upgradeButtonLabel}}
class="upgrade-button"
/>
</div>
<DButton
@action={{this.upgrade}}
@disabled={{this.upgradeDisabled}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Another note that all of this upgrade wording in the code and CSS should be changed to update.

@translatedLabel={{this.upgradeButtonLabel}}
class="upgrade-button"
/>
{{/if}}
</td>
</tr>
56 changes: 54 additions & 2 deletions admin/assets/javascripts/discourse/models/repo.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { tracked } from "@glimmer/tracking";
import { cached, tracked } from "@glimmer/tracking";
import { capitalize } from "@ember/string";
import { TrackedObject } from "@ember-compat/tracked-built-ins";
import { ajax } from "discourse/lib/ajax";
import I18n from "discourse-i18n";
import AdminPlugin from "admin/models/admin-plugin";

let loaded = [];
export let needsImageUpgrade = false;
Expand Down Expand Up @@ -74,6 +77,7 @@ export default class Repo {
@tracked checking = false;
@tracked lastCheckedAt = null;
@tracked latest = new TrackedObject({});
@tracked plugin = null;

// model attributes
@tracked name = null;
Expand All @@ -94,15 +98,59 @@ export default class Repo {
}
}

if (attributes.plugin) {
this.plugin = AdminPlugin.create(attributes.plugin);
}

for (const [key, value] of Object.entries(attributes)) {
if (key === "latest") {
if (["latest", "plugin"].includes(key)) {
continue;
}

this[key] = value;
}
}

@cached
get nameTitleized() {
if (this.plugin) {
return this.plugin.nameTitleized;
}

let name = this.name
.split(/[-_]/)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just wondering why you need this other stuff? Everything on this page is either Discourse or a plugin isn't it?

.map((word) => {
return capitalize(word);
})
.join(" ");

// Cuts down on repetition.
const discoursePrefix = "Discourse ";
if (name.startsWith(discoursePrefix)) {
name = name.slice(discoursePrefix.length);
}

return name;
}

get linkUrl() {
if (this.plugin) {
return this.plugin.linkUrl;
}

return this.url;
}

get author() {
if (this.plugin) {
return this.plugin.author;
} else if (this.name === "docker_manager") {
return I18n.t("admin.plugins.author", { author: "Discourse" });
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary -- docker_manager is an official plugin owned by us, so it should do this by default with this.plugin.author:

https://github.com/discourse/discourse/blob/792e66966c3006ec4abe9746d1554a44fcbe0c2c/app/assets/javascripts/admin/addon/models/admin-plugin.js#L88-L94

}

return null;
}

get checkingStatus() {
return this.unloaded || this.checking;
}
Expand All @@ -111,6 +159,10 @@ export default class Repo {
return !this.upgrading && this.version === this.latest?.version;
}

get hasNewVersion() {
return !this.checkingStatus && !this.upToDate;
}

get prettyVersion() {
return this.pretty_version || this.version?.substring(0, 8);
}
Expand Down
42 changes: 24 additions & 18 deletions admin/assets/javascripts/discourse/templates/update-index.hbs
Original file line number Diff line number Diff line change
@@ -1,4 +1,21 @@
<h1>{{i18n "admin.docker.update_title"}}</h1>
<div class="updates-heading">
<h3>{{i18n "admin.docker.update_title"}}</h3>
{{#unless this.outdated}}
<button
disabled={{this.upgradeAllButtonDisabled}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use DButton for this? This one has currently got slightly different styling to the Update buttons in the list

id="upgrade-all"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should definitely be changing the code variables and functions from upgrade to update as well, otherwise it is confusing

Copy link
Contributor

Choose a reason for hiding this comment

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

And CSS classes

class="btn btn-primary"
type="button"
{{on "click" this.upgradeAllButton}}
>
{{#if this.allUpToDate}}
{{i18n "admin.docker.all_up_to_date"}}
{{else}}
{{i18n "admin.docker.update_all"}}
{{/if}}
</button>
{{/unless}}
</div>

{{#if this.outdated}}
<h2>{{i18n "admin.docker.outdated_image_header"}}</h2>
Expand All @@ -18,25 +35,14 @@
</a>
</p>
{{else}}
<button
disabled={{this.upgradeAllButtonDisabled}}
id="upgrade-all"
class="btn"
type="button"
{{on "click" this.upgradeAllButton}}
>
{{#if this.allUpToDate}}
{{i18n "admin.docker.all_up_to_date"}}
{{else}}
{{i18n "admin.docker.update_all"}}
{{/if}}
</button>

<table class="table" id="repos">
<table class="table admin-repos" id="repos">
<thead>
<th></th>
<th style="width: 50%">{{i18n "admin.docker.repository"}}</th>
<th>{{i18n "admin.docker.status"}}</th>
<th style="width: 40%">{{i18n "admin.docker.repo.name"}}</th>
<th>{{i18n "admin.docker.repo.commit_hash"}}</th>
<th>{{i18n "admin.docker.repo.last_updated"}}</th>
<th>{{i18n "admin.docker.repo.latest_version"}}</th>
<th align="center">{{i18n "admin.docker.repo.status"}}</th>
</thead>
<tbody>
{{#each this.model as |repo|}}
Expand Down
7 changes: 7 additions & 0 deletions app/controllers/docker_manager/admin_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ def repos
official: Plugin::Metadata::OFFICIAL_PLUGINS.include?(r.name),
}

plugin = Discourse.visible_plugins.find { |p| p.path == "#{r.path}/plugin.rb" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to be careful with this...the page errors completely on the client if you give it a wrong path:

2024-05-03_11-46

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe this is an existing problem -- either way we should account for this.

result[:plugin] = AdminPluginSerializer.new(
plugin,
scope: guardian,
root: false,
) if plugin.present?

result[:fork] = true if result[:official] &&
!r.url.starts_with?("https://github.com/discourse/")

Expand Down
67 changes: 63 additions & 4 deletions assets/stylesheets/common/docker-manager.scss
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,6 @@
}
}

#upgrade-all {
float: right;
}

#banner {
margin: 1rem 0;

Expand Down Expand Up @@ -147,4 +143,67 @@
}
}
}

.repo__name {
font-weight: bold;
}

.repo__author {
font-size: var(--font-down-2);
padding: 0 0 0.25em 0;
}

.repo__new-version {
font-size: var(--font-down-1);
font-weight: bold;
padding: 0.25em 0 0 0;
}

.repo__about {
padding: 0.25em 0 0.25em 0;

a {
color: var(--primary-700);
text-decoration: underline;
}
}

ul.repo__latest-version {
list-style: none;
margin: 0;
}

tr.repo {
td {
padding: 1em 0;
}

&.new-version {
background-color: var(--tertiary-very-low);

td:first-child {
border-left: solid 3px var(--tertiary);
}
}

td.repo__status {
text-align: right;
padding-right: 0.5em;
}
}

.updates-heading {
display: flex;
justify-content: space-between;
margin: 2em 0;

h3 {
line-height: 40px;
margin-bottom: 0;
}
}

.admin-repos th:last-child {
text-align: center;
}
}
8 changes: 7 additions & 1 deletion config/locales/client.en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,14 @@ en:
update_repo: "Update %{name}"
update_successful: "Update completed successfully!"
update_tab: "Update Discourse"
update_title: "Update Discourse"
update_title: "Updates"
updating: "Updating…"
repo:
name: "Name"
commit_hash: "Commit Hash"
last_updated: "Last Updated"
latest_version: "Latest Version"
status: "Status"

logs:
staff_actions:
Expand Down