Skip to content

Conversation

@cpcallen
Copy link
Collaborator

@cpcallen cpcallen commented Jul 3, 2025

Resolves

Fixes #595.

Proposed Changes

Move styling of:

  • Comment delete icon
  • Focus indicators
  • Context menu items

from test/index.html to src/index.ts.

Additionally, via RaspberryPiFoundation/blockly#9201, move a fix for the toolbox focus styling from the plugin to core.

Reason for Changes

It should not be necessary to manually add styling to projects using the @blockly/keyboard-navigation plugin in order for the plugin to function as expectd.

Test Coverage

Manually verified that there were no obvious styling differences after making these changes.

Additional Information

Much of the styling added to src/index.ts, particularly the focus-related styling, should be moved to core. Unfortunately because it intersects with the existing styling of rendered elements, it should probably be done by breaking it up and moving it to appropriate places in core/renderers/, rather than just dumping it in core/css.ts, and I'm just not sure where it should all go yet, so in the interest of expediency I am satisfying #595 and will file a separate issue for the move to core.

cpcallen added 6 commits July 2, 2025 13:18
The blocklyToolboxDiv class was renamed blocklyToolbox in
RaspberryPiFoundation/blockly#8647, fixing RaspberryPiFoundation/blockly#8343.

The references in this repository were not updated but are now
evidently obsolete: if we wanted them we'd have fixed them by now.
Also apply only to .blocklyToolbox instead of *
Also move them from html to .injectionDiv.
@cpcallen cpcallen requested a review from a team as a code owner July 3, 2025 00:49
@cpcallen cpcallen requested review from gonfunko and removed request for a team July 3, 2025 00:49
@cpcallen
Copy link
Collaborator Author

cpcallen commented Jul 3, 2025

@microbit-matt-hillsdon: Please take a look at this and RaspberryPiFoundation/blockly#9201.

@cpcallen
Copy link
Collaborator Author

cpcallen commented Jul 7, 2025

I can't enable auto-squash on this but am happy to have it merged on my behalf once successfully reviewed.

@cpcallen cpcallen changed the title refactor(CSS): move plugin styling from test/index.html to src/index.js. refactor(CSS): move plugin styling from test/index.html to src/index.js Jul 7, 2025
@gonfunko
Copy link
Contributor

gonfunko commented Jul 7, 2025

Rather than having this run at load, can the registering be moved into a (static?) method registerKeyboardNavigationStyles() or the like? We've regularly run into issues with accidental re-registering of styles, which IIRC throws if it happens after inject() is called, and also issues with multiple KeyboardNavigation instances.

@rachel-fenichel
Copy link
Collaborator

Rather than having this run at load, can the registering be moved into a (static?) method registerKeyboardNavigationStyles() or the like? We've regularly run into issues with accidental re-registering of styles, which IIRC throws if it happens after inject() is called, and also issues with multiple KeyboardNavigation instances.

If I recall correctly, this has to be run at load (or in a static method called on load) because you have to call Css.register before inject.

@gonfunko
Copy link
Contributor

gonfunko commented Jul 7, 2025

That's correct, but I think confining it to a static method that needs to be called will avoid the other issues and is helpful on net.

@rachel-fenichel
Copy link
Collaborator

Okay, Maribeth asked me to rebase this and make that change, so I'm also assigning it to myself.

@rachel-fenichel
Copy link
Collaborator

Replaced by #650 (to complete the change while Christopher is offline).

@cpcallen cpcallen deleted the refactor/595 branch July 10, 2025 08:22
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.

include necessary styles in plugin source code

3 participants