Show owner list with full names, add an asterisk for mfa disable when you are owner #4905
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4905 +/- ##
=======================================
Coverage 97.03% 97.03%
=======================================
Files 421 421
Lines 8776 8783 +7
=======================================
+ Hits 8516 8523 +7
Misses 260 260 ☔ View full report in Codecov by Sentry. |
app/helpers/rubygems_helper.rb
Outdated
| link_text << " #{owner.display_handle}" | ||
| link_text << " *" if show_mfa_status && owner.mfa_disabled? | ||
| link_to( | ||
| link_text.html_safe, |
There was a problem hiding this comment.
let's put the link text in a localized html string?
There was a problem hiding this comment.
There's no actual text so this would just be:
owner_link_html: "%{avatar} %{handle}%{mfa_unable}"This is just string interpolation so we should avoid putting a lot of distance between caller and the format, imho.
There was a problem hiding this comment.
wouldn't the order still be different in an RTL language?
There was a problem hiding this comment.
I'd like to avoid overthinking it for now. I'm not sure that star placement is going to ruin a RTL language. We don't have any yet anyway, so that will be its own large undertaking, of which this will be a minor detail.
martinemde
force-pushed
the
martinemde/owner-list-with-name-and-mfa-status
branch
from
1407f6d
to
1d13ba7
Compare
martinemde
force-pushed
the
martinemde/owner-list-with-name-and-mfa-status
branch
from
192ce06
to
f890730
Compare
martinemde
force-pushed
the
martinemde/owner-list-with-name-and-mfa-status
branch
from
f890730
to
d85468f
Compare
Not quite complete, but looking for feedback. This was much easier than fixing the broken javascript (not that I didn't try) and solves a policy problem in a cleaner way.