Skip to content

Conversation

Nateowami
Copy link
Collaborator

@Nateowami Nateowami commented Oct 8, 2025

Creating this PR a little before we're actually ready to merge it (there are outstanding changes to merge before we enable this feature), in hopes of having it reviewed and ready to test sooner.


This change is Reviewable

@Nateowami Nateowami added will require testing PR should not be merged until testers confirm testing is complete critical path PRs that are on our critical path and need attention to keep things moving. Priority 2nd. labels Oct 8, 2025
Copy link

codecov bot commented Oct 8, 2025

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3499      +/-   ##
==========================================
- Coverage   82.31%   82.30%   -0.01%     
==========================================
  Files         613      613              
  Lines       36877    36877              
  Branches     6022     6041      +19     
==========================================
- Hits        30354    30352       -2     
+ Misses       5642     5629      -13     
- Partials      881      896      +15     

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

@pmachapman pmachapman self-assigned this Oct 8, 2025
@pmachapman pmachapman self-requested a review October 8, 2025 22:18
Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

Looks good. Just one minor comment!

@pmachapman reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pmachapman)


src/SIL.XForge.Scripture/ClientApp/src/app/navigation/navigation.component.scss line 99 at r1 (raw file):

}

.nav-label {

.nav-label also appears in the block:

  &.activated-nav-item,
  &.active {
    background-color: var(--sf-navigation-list-item-active-background-color);
    --mdc-list-list-item-label-text-color: var(--sf-navigation-list-item-active);
    --mdc-list-list-item-hover-label-text-color: var(--sf-navigation-list-item-active);
    --mdc-list-list-item-focus-label-text-color: var(--sf-navigation-list-item-active);
    .nav-label {
      background-color: var(--sf-navigation-list-item-active);
      color: var(--sf-navigation-list-item-active-background-color);
    }
  }

Do you want to remove it there also?

Code quote:

.nav-label {

@pmachapman pmachapman added the wait for next release Should not be merged until after the next release label Oct 8, 2025
@Nateowami
Copy link
Collaborator Author

Thanks for the catch. Why did you add wait for next release? We're waiting on another PR, not the next release.

@Nateowami Nateowami force-pushed the feature/SF-3597-enable-draft-options branch from 303e22e to 1eac8a5 Compare October 8, 2025 22:30
Copy link
Collaborator Author

@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.

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @pmachapman)


src/SIL.XForge.Scripture/ClientApp/src/app/navigation/navigation.component.scss line 99 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

.nav-label also appears in the block:

  &.activated-nav-item,
  &.active {
    background-color: var(--sf-navigation-list-item-active-background-color);
    --mdc-list-list-item-label-text-color: var(--sf-navigation-list-item-active);
    --mdc-list-list-item-hover-label-text-color: var(--sf-navigation-list-item-active);
    --mdc-list-list-item-focus-label-text-color: var(--sf-navigation-list-item-active);
    .nav-label {
      background-color: var(--sf-navigation-list-item-active);
      color: var(--sf-navigation-list-item-active-background-color);
    }
  }

Do you want to remove it there also?

Done.

@pmachapman
Copy link
Collaborator

pmachapman commented Oct 8, 2025

Why did you add wait for next release? We're waiting on another PR, not the next release.

@Nateowami Just to stop an accidental merge by someone else. You can remove it if you like (it was the closest tag I could think of to the concept of waiting for another PR) I have removed it...the will require testing is sufficient...sorry I was overthinking!

@pmachapman pmachapman removed the wait for next release Should not be merged until after the next release label Oct 8, 2025
@Nateowami
Copy link
Collaborator Author

@pmachapman Should we have a "don't merge" label and then explain why in the PR?

@pmachapman
Copy link
Collaborator

Should we have a "don't merge" label and then explain why in the PR?

We can...Maybe the chat history will be enough! Sorry for the distraction!

@Nateowami Nateowami added the do not merge See PR description and/or comments for explanation label Oct 8, 2025
@Nateowami
Copy link
Collaborator Author

Not a problem; label created.

Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
critical path PRs that are on our critical path and need attention to keep things moving. Priority 2nd. do not merge See PR description and/or comments for explanation will require testing PR should not be merged until testers confirm testing is complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants