Skip to content

Conversation

StephenMcConnel
Copy link
Contributor

@StephenMcConnel StephenMcConnel commented Sep 29, 2025

This change is Reviewable

@StephenMcConnel StephenMcConnel marked this pull request as draft September 29, 2025 22:10
@StephenMcConnel
Copy link
Contributor Author

This is a draft until BloomBooks/bloom-harvester#228 has been approved, merged, and deployed.

@StephenMcConnel StephenMcConnel force-pushed the BL-15289-ShowSpecificReasonsForHarvesterHiding branch from 268fe91 to 3989b16 Compare October 2, 2025 17:14
Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

@andrew-polk reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @StephenMcConnel)


src/model/ArtifactVisibilitySettings.ts line 111 at r1 (raw file):

                            id: this.harvesterReasonToHideId
                                ? this.harvesterReasonToHideId
                                : "book.artifacts.visibility.harvesterHidden",

If the default message stays the same, should the ID be the same?

Either way, I think we want these in sync with Code String.json.

@andrew-polk
Copy link
Contributor

src/model/ArtifactVisibilitySettings.ts line 111 at r1 (raw file):

Previously, andrew-polk wrote…

If the default message stays the same, should the ID be the same?

Either way, I think we want these in sync with Code String.json.

In other words, you changed the ID here but not there. But I think it makes sense to just leave the ID the same if we are not changing the string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants