Skip to content

Conversation

@marksvc
Copy link
Collaborator

@marksvc marksvc commented Oct 28, 2025

Ideally we can show the content on one half of a 1920px wide screen. The full Serval build id is retained, which allows it to be searched on the page with Ctrl+F.

Un-wrapped:

image

One half of 1920px:

image

This change is Reviewable

@marksvc
Copy link
Collaborator Author

marksvc commented Oct 28, 2025

Set as 'testing-not-required'. I don't think it needs to use time there.

@codecov
Copy link

codecov bot commented Oct 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.91%. Comparing base (2e41d44) to head (03bf05d).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3545   +/-   ##
=======================================
  Coverage   82.91%   82.91%           
=======================================
  Files         605      605           
  Lines       36876    36876           
  Branches     6040     6045    +5     
=======================================
  Hits        30574    30574           
  Misses       5376     5376           
  Partials      926      926           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@marksvc marksvc marked this pull request as ready for review October 28, 2025 19:17
Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

@Nateowami reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @marksvc)


-- commits line 2 at r1:
Can you write a clearer title?


src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-jobs.component.html line 137 at r1 (raw file):

          <th mat-header-cell *matHeaderCellDef>Author</th>
          <td mat-cell *matCellDef="let row">
            <app-owner [ownerRef]="row.userId" [includeAvatar]="true"></app-owner>

I'm surprised you removed the timestamp from the owner component, rather removing the timestamp column from the table.

Your screenshots exaggerate how big of a difference the removal of the timestamp from the owner component makes, since they all say "me", whereas on production they won't (it still will make a big difference, just not as big).

Either way, I'd think you could reduce the width more through removing the timestamp column.

@Nateowami Nateowami self-assigned this Oct 28, 2025
Copy link
Collaborator Author

@marksvc marksvc left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @Nateowami)


-- commits line 2 at r1:

Previously, Nateowami wrote…

Can you write a clearer title?

Yes. Depending on what you mean. Are you looking for a nuts-and-bolts statement like
"Remove date from and wrap Owner and build id, hide Status label on narrower screens in SA draft jobs"?

Or are you meaning "sa" is not clear? So,
"system administration: display draft jobs on narrower screen"

Or what are you meaning?


src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-jobs.component.html line 137 at r1 (raw file):

Previously, Nateowami wrote…

I'm surprised you removed the timestamp from the owner component, rather removing the timestamp column from the table.

Your screenshots exaggerate how big of a difference the removal of the timestamp from the owner component makes, since they all say "me", whereas on production they won't (it still will make a big difference, just not as big).

Either way, I'd think you could reduce the width more through removing the timestamp column.

I think of timestamp as one of the more important columns :-). But I guess it depends on what someone is looking for.
I would be inclined to change the timestamp to something more tidy, like

2025-12-31
12:34:56Z

or

2025-12-31
06:34:56 -0600

but I notice that no where else do we do that.

I don't want the timestamp to appear in the Owner component because it seems confusing to show the job start time as part of the user identifier. (It seems like a date there should mean when the user created their account or something.)

I put in a longer name for layout testing. What do you think about the layout shown now?

(0.5*1920px width shown)
image.png

@marksvc
Copy link
Collaborator Author

marksvc commented Oct 29, 2025

-- commits line 2 at r1:

Previously, marksvc wrote…

Yes. Depending on what you mean. Are you looking for a nuts-and-bolts statement like
"Remove date from and wrap Owner and build id, hide Status label on narrower screens in SA draft jobs"?

Or are you meaning "sa" is not clear? So,
"system administration: display draft jobs on narrower screen"

Or what are you meaning?

Oh, "sa: fit draft jobs info or narrower screen"?

@marksvc marksvc changed the title sa: display draft jobs on narrower display sa: fit draft jobs list on narrower display Oct 29, 2025
Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

@Nateowami reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @marksvc)


-- commits line 2 at r1:

Previously, marksvc wrote…

Oh, "sa: fit draft jobs info or narrower screen"?

[sorry; I wrote a followup but forgot to post it]

To elaborate: at a minimum, "sa" is ambiguous and unclear what it refers to. We have "system administration" and "serval administration" areas. I would probably suggest something like "Improve layout of draft job list for small screens" if you want to keep it really short, and include "Serval admin" if you don't care about being as brief as possible (I don't think brevity is that important).

Copy link
Collaborator Author

@marksvc marksvc left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Nateowami)


-- commits line 2 at r1:

Previously, Nateowami wrote…

[sorry; I wrote a followup but forgot to post it]

To elaborate: at a minimum, "sa" is ambiguous and unclear what it refers to. We have "system administration" and "serval administration" areas. I would probably suggest something like "Improve layout of draft job list for small screens" if you want to keep it really short, and include "Serval admin" if you don't care about being as brief as possible (I don't think brevity is that important).

Thank you :). I wasn't even thinking about the "SA" ambiguity. And I see I even got mixed up in one of my examples in this thread.

@marksvc marksvc changed the title sa: fit draft jobs list on narrower display Serval admin: Improve layout of draft job list for small screens Nov 3, 2025
@marksvc marksvc added the e2e Run e2e tests for this pull request label Nov 3, 2025
Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

:lgtm:

@Nateowami reviewed 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @marksvc)

@Nateowami Nateowami merged commit 6d7adb5 into master Nov 4, 2025
23 checks passed
@Nateowami Nateowami deleted the task/sa-narrower branch November 4, 2025 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

e2e Run e2e tests for this pull request testing not required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants