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

SF-3124 Add sync message before draft queued message #2923

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

josephmyers
Copy link
Collaborator

@josephmyers josephmyers commented Dec 19, 2024

To do this, we check the queue count in the project doc and display that message if the build job state is queued. Once the sync count is zero, we display the regular queue message.


This change is Reviewable

@josephmyers
Copy link
Collaborator Author

I said testing is not required, but feel free to undo that

@josephmyers josephmyers marked this pull request as ready for review December 19, 2024 08:03
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.90%. Comparing base (ec1d939) to head (65a356d).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2923   +/-   ##
=======================================
  Coverage   80.90%   80.90%           
=======================================
  Files         533      533           
  Lines       31204    31205    +1     
  Branches     5060     5060           
=======================================
+ Hits        25246    25247    +1     
  Misses       5210     5210           
  Partials      748      748           

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

@kylebuss kylebuss self-assigned this Dec 23, 2024
Copy link
Collaborator

@kylebuss kylebuss left a comment

Choose a reason for hiding this comment

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

Nice job Joseph! One small comment/suggestion.

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


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.html line 280 at r1 (raw file):

                  @if (isSyncing()) {
                    <h3>{{ t("draft_syncing") }}</h3>
                    <p>{{ t("draft_syncing_detail") }}</p>

We could show the app-sync-progress bar here instead of the detail message and gears.

Code quote:

<p>{{ t("draft_syncing_detail") }}</p>

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 191 at r1 (raw file):

    "draft_is_ready": "Your draft is ready",
    "draft_syncing": "Draft initializing",
    "draft_syncing_detail": "Your project is being synced before queuing the draft.",

If you do decide to switch to the sync progress bar "draft_syncing" could say something like "Syncing projects".

Code quote:

    "draft_syncing": "Draft initializing",
    "draft_syncing_detail": "Your project is being synced before queuing the draft.",

Copy link
Collaborator Author

@josephmyers josephmyers left a comment

Choose a reason for hiding this comment

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

Hey, this is a really interesting idea. Way to think outside the box! I've been considering this for a while today and looking over what it would mean for the UX, and I think we should forego this, for the following reasons:

  • The gears serve as an open-ended (critically), introductory animation that communicates the setup state quite well.
  • If we instead start everything off with a progress bar, they will associate that bar with the draft, whether or not we have text that says otherwise, just because they clicked the Start button and now they see a progress bar.
  • Further, it would be a little strange to go from a progress bar, to the gears, then to a completely different progress bar.

I do appreciate the brainstorming, but I think we should pass, though I am quite open to your thoughts on this. We do have other, bigger ways we could change things, e.g. integrating all three phases into the circle progress bar, but I don't see a ton of value for the time it would cost.

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

Copy link
Collaborator

@kylebuss kylebuss left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @josephmyers)

@josephmyers josephmyers enabled auto-merge (squash) January 3, 2025 01:49
To do this, we check the queue count in the project doc and display that message if the build job state is queued. Once the sync count is zero, we display the regular queue message.
@josephmyers josephmyers merged commit 3ef90cf into master Jan 6, 2025
14 of 15 checks passed
@josephmyers josephmyers deleted the improvement/SF-3124 branch January 6, 2025 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants