Skip to content

Conversation

@gonfunko
Copy link
Contributor

@gonfunko gonfunko commented Jun 27, 2025

In conjunction with RaspberryPiFoundation/blockly#9182, this PR adds support for editing workspace comments, activating their icons via keyboard navigation, and moving them.

@gonfunko gonfunko requested a review from a team as a code owner June 27, 2025 21:16
@gonfunko gonfunko requested review from BenHenning and removed request for a team June 27, 2025 21:16
Copy link
Collaborator

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Generally looks good, though is it possible to add tests for both comment cases?

@BenHenning BenHenning assigned gonfunko and unassigned BenHenning Jul 1, 2025
@gonfunko
Copy link
Contributor Author

gonfunko commented Jul 1, 2025

This PR fixes #614.

@gonfunko
Copy link
Contributor Author

gonfunko commented Jul 2, 2025

Tests have been added.

Copy link
Collaborator

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @gonfunko. LGTM, though happy to take another pass if you'd like. I did have a few suggestions, but I think they're all hopefully non-controversial.

chai.assert.equal(focusedNodeId, `${this.comment1}_collapse_bar_button`);
});

test('Activate workspace comment button', async function () {
Copy link
Collaborator

@BenHenning BenHenning Jul 2, 2025

Choose a reason for hiding this comment

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

Maybe add a test for toggling to show that it can be expanded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that'd be better handled in the core workspace comment tests; my goal here is just to ensure that the keyboard nav infrastructure is hooked up to the underlying views/buttons.

@microbit-matt-hillsdon
Copy link
Contributor

microbit-matt-hillsdon commented Jul 3, 2025

Feedback from @microbit-grace testing workspace comments nav today:

  • there's a tab stop for a new workspace comment textarea immediately after creation, is that still intended?
  • we think the constrained/unconstrained distinction doesn't make sense for workspace comments so move mode should just do unconstrained without a modifier
  • focus styles are particularly unclear (but can fix in MakeCode CSS)

@microbit-matt-hillsdon
Copy link
Contributor

I've used the following styles in MakeCode on our preview branch which includes this PR. Maybe use the same blue as for --blockly-active-tree-color in the plugin if there's enough contrast (MakeCode uses black for that due to a blue top nav bar).

/* Comments are yellow default highlight is orange */
--blockly-active-workspace-comment-color: black;

...

/* Workspace comments */
.blocklyKeyboardNavigation .blocklyComment.blocklyActiveFocus .blocklyCommentHighlight {
    stroke: var(--blockly-active-workspace-comment-color);
    stroke-width: 4px;
}
.blocklyCommentTopbar .blocklyActiveFocus {
    outline: 2px solid var(--blockly-active-workspace-comment-color);
    border-radius: 2px;
    outline-offset: -1px;
}
image image

Yellow on yellow or the orange outline seem too subtle.

@microbit-matt-hillsdon
Copy link
Contributor

Also noticed that comments are not scrolled into view when you navigate to them.

@gonfunko
Copy link
Contributor Author

gonfunko commented Jul 7, 2025

Thanks for the feedback! Agreed that all broadly sounds reasonable; I'll merge this as-is to unblock #626 and address those points in a followup later today.

@gonfunko gonfunko merged commit 8714c2c into main Jul 7, 2025
8 checks passed
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.

5 participants