Skip to content

fix: remove code of removed colour toolbox#2308

Merged
maribethb merged 2 commits intoRaspberryPiFoundation:masterfrom
mobyw:master
Apr 4, 2024
Merged

fix: remove code of removed colour toolbox#2308
maribethb merged 2 commits intoRaspberryPiFoundation:masterfrom
mobyw:master

Conversation

@mobyw
Copy link
Contributor

@mobyw mobyw commented Apr 4, 2024

The basics

The details

Resolves

Fixes error loading caused by applying changes in #2294.

Proposed Changes

Remove JavasSript and CSS codes of the removed colour toolbox.

@mobyw mobyw requested a review from a team as a code owner April 4, 2024 09:16
@mobyw mobyw requested review from BeksOmega and removed request for a team April 4, 2024 09:16
Copy link
Contributor

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

I don't think this demo is using Blockly quite correctly, but it's fine to merge this to unblock other work.

toolboxJson['contents'][4].name = getMsg('Lists');
toolboxJson['contents'][5].name = getMsg('Colour');
// Separator.
toolboxJson['contents'][7].name = getMsg('Variables');
Copy link
Contributor

Choose a reason for hiding this comment

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

@maribethb do you know why we're doing this instead of using Blockly's normal localization?

Copy link
Contributor

Choose a reason for hiding this comment

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

Toolboxes don't really have support for dynamic messages afaik. I could see a couple different ways of solving this but I'm not sure which method you're referring to as "normal."

If you want history though, you might have to check with Neil. This is the old demo that's been on devsite for forever and I'm not sure where it came from before we moved it to samples.

Copy link
Contributor

Choose a reason for hiding this comment

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

They support replacing message references in category names. Although I guess we don't document this anywhere?

@NeilFraser do you have context for this?

Copy link
Contributor

@maribethb maribethb Apr 4, 2024

Choose a reason for hiding this comment

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

TIL, I thought you had to do this manually. I thought one of the old demos was doing something similar but I can't find it.

#blockly-8 {
#blockly-7 {
border-left: 15px solid #d7ccc8 !important;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these CSS IDs are technically supported. We should be using a custom toolbox, or category CSS.

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 tried using category css and it works.

Should I update on this PR or open another one when this is done?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can make updates on this PR! Thanks for testing that out :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @mobyw since this fixes the issue with the live demo we'll actually go ahead and merge it now so we can make sure the demo is running. If you're still up for fixing the css to use the category CSS, that would be great and we'd love a follow up PR! Thanks!

@maribethb maribethb merged commit d9caf5d into RaspberryPiFoundation:master Apr 4, 2024
gonfunko pushed a commit to gonfunko/blockly-samples that referenced this pull request Apr 15, 2024
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.

3 participants