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.

Sign up for GitHub

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

Jump to bottom

Improve UX on modal for deleting an access token #19894

Merged
merged 5 commits into from
Jun 7, 2022
Merged

Improve UX on modal for deleting an access token #19894

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5a2add6
Improve UX on modal for deleting an access token
Ryuno-Ki Jun 5, 2022
7a877a3
Use tabs instead of spaces.
Ryuno-Ki Jun 5, 2022
09ff43e
Merge branch 'main' into ux-3589-delete-access-token-modal
wxiaoguang Jun 6, 2022
2c01ac0
Merge branch 'main' into ux-3589-delete-access-token-modal
wxiaoguang Jun 7, 2022
0085767
Merge branch 'main' into ux-3589-delete-access-token-modal
techknowlogick Jun 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions templates/user/settings/applications.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
<div class="content">
<strong>{{.Name}}</strong>
<div class="activity meta">
<i>{{$.i18n.Tr "settings.add_on"}} <span>{{.CreatedUnix.FormatShort}}</span> — {{svg "octicon-info"}} {{if .HasUsed}}{{$.i18n.Tr "settings.last_used"}} <span {{if .HasRecentActivity}}class="green"{{end}}>{{.UpdatedUnix.FormatShort}}</span>{{else}}{{$.i18n.Tr "settings.no_activity"}}{{end}}</i>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can back this out if required. But double spaces itched me here. Is it about giving the icon some space?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not unusual to sneak in some small code-style into a PR.

<i>{{$.i18n.Tr "settings.add_on"}} <span>{{.CreatedUnix.FormatShort}}</span> — {{svg "octicon-info"}} {{if .HasUsed}}{{$.i18n.Tr "settings.last_used"}} <span {{if .HasRecentActivity}}class="green"{{end}}>{{.UpdatedUnix.FormatShort}}</span>{{else}}{{$.i18n.Tr "settings.no_activity"}}{{end}}</i>
</div>
</div>
</div>
Expand Down Expand Up @@ -55,6 +55,12 @@
</div>

<div class="ui small basic delete modal" id="delete-token">
{{/*
The wording here is a bit confusing.
The action asks for deleting an access token.
Confirming is therefore the destructive option and
should be deemphasized because it cannot be undone.
*/}}
Comment on lines +58 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we leave comments like this in the code. Haven't seen it before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was on the fence. But then looking at the template below where „yes” was in red and no was marked up I thought it warranted an explanation.

Now I don't want to make contributors hunt the code history to find the motivation. And the comment wouldn't get rendered out (I checked). Aside, Go files have comments for functions.

I don't mind if I shall remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tbh would be better if the comment wouldn't be rendered to the client(if anyone wants to find this motivation, opting to go search it in the code should be de-facto default).

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the template comments are rendered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They aren't. I double checked.

<div class="ui icon header">
{{svg "octicon-trash"}}
{{.i18n.Tr "settings.access_token_deletion"}}
Expand All @@ -63,11 +69,11 @@
<p>{{.i18n.Tr "settings.access_token_deletion_desc"}}</p>
</div>
<div class="actions">
<div class="ui red basic inverted cancel button">
<div class="ui cancel button">
<i class="remove icon"></i>
{{.i18n.Tr "modal.no"}}
</div>
<div class="ui green basic inverted ok button">
<div class="ui red basic inverted ok button">
<i class="checkmark icon"></i>
{{.i18n.Tr "modal.yes"}}
Comment on lines 74 to 78
Copy link
Member

Choose a reason for hiding this comment

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

If we are improving UX already, wouldn't it be better to use No -> Cancel and Yes -> Delete instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would trigger a change in the translation. I wrote my thoughts in #3589 (comment)

Therefore, the PR only mentions, it is associated with the issue, i.e. not a complete fix.

How long does it take to complete a translation cycle?
I would fill a follow-up PR to adjust the wording (as I mentioned, I would highlight that the change cannot be undone in this case). Inconsistency in modals are also discussed in #3605

I'm not sure, whether the Template should become a Vue component, for example. Or be made reusable.
I can see arguments pro and con.

Copy link
Contributor

@wxiaoguang wxiaoguang Jun 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

"How long does it take to complete a translation cycle?"

PR changes the locale_en-US.ini -> PR gets merged -> 24h (at most) -> text gets translated in crowdin.com -> translation gets approved -> 24h (at most) -> (auto) commit & push the translation.

So, if there are active proof-readers, in (at most) 48h the translations can be updated, and there is still enough time before Gitea 1.17's release.

ps: I think the text could be updated directly without considering the translation cycle. Personally I always update texts if they need to be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given https://github.com/go-gitea/gitea/blob/main/CONTRIBUTING.md#code-review

Make small pull requests. The smaller, the faster to review and the more likely it will be merged soon.

I will brew a follow-up PR with the wording changes.

Copy link
Member

Choose a reason for hiding this comment

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

This PR is already incredibly small.
This is rather meant for bigger changes, i.e. PRs that change 400+ lines.

</div>
Expand Down