google / blockly

The web-based visual programming editor.
https://developers.google.com/blockly/
Apache License 2.0
12.4k stars 3.7k forks source link

Make it possible for shadow blocks to mirror the parent colour / style #7607

Open BeksOmega opened 11 months ago

BeksOmega commented 11 months ago

Check for duplicates

Problem

This feature request was first filed in https://github.com/google/blockly/issues/7416

See here for a good visual and explanation: https://github.com/google/blockly/issues/7416#issuecomment-1770149878

Request

The submitted request is to add a constant to the renderer (BLOCKS_USE_PARENT_STYLE) that determines whether shadow blocks should match the colors of their parent block.

Note that this would be a workspace-global setting, and not toggleable on a block-specific basis. So you couldn't have text blocks match their parents while special dropdown blocks don't.

Alternatives considered

I don't think continuously adding settings to themes and renderers to get the to handle complex styling behavior is sustainable. It also breaks separation of responsibility, by forcing renderers to care a bunch about themes.

I think the proper thing to do here is to add more CSS support for themes, so we can leverage the browser's built-in ability to do complex styling. I /think/ we can change themes under the hood to register css instead of setting fill attributes. But this needs further investigation (i.e. a design doc).

Additional context

No response

laurensvalk commented 11 months ago

I can see why growing the number of config options could be undesirable. Perhaps another option may be to provide the required handles in the renderer and leave it up to the application. (It's also quite possible that they are already there :smile:)

For what it's worth, I've currently done it this way. It has a few monkey patches and covers only the fields I've used.

class CustomPathObject extends Blockly.zelos.PathObject {
    override applyColour(block: Blockly.BlockSvg) {
        super.applyColour(block);

        // Set shadow fill colour to match parent; stroke already set by super.
        const parent = block.getParent();
        if (block.isShadow() && parent) {
            this.svgPath.setAttribute('fill', parent.style.colourSecondary);
        }
    }
}

export const pybricksRendererName = 'pybricks_renderer';

class PybricksRenderer extends Blockly.zelos.Renderer {
    constructor() {
        super(pybricksRendererName);
    }

    // Done to facilitate other renderer changes; not needed for the color override.
    makeConstants_(): CustomConstantProvider {
        return new CustomConstantProvider();
    }

    override makePathObject(
        root: SVGElement,
        style: Blockly.Theme.BlockStyle,
    ): CustomPathObject {
        return new CustomPathObject(
            root,
            style,
            this.getConstants() as CustomConstantProvider,
        );
    }
}

Blockly.blockRendering.register(pybricksRendererName, PybricksRenderer);

// In the renderer, we make shadow blocks inherit their fill from their parent.
// This doesn't address the borderrect, so we have to monkey patch it. This
// is used to patch each of the fields we use below.
const updateBorderRect = (source: Blockly.BlockSvg, borderRect: SVGRectElement) => {
    const parent = source.getParent();
    if (borderRect && parent && source.isShadow()) {
        borderRect.setAttribute('stroke', parent.style.colourTertiary);
    }
};

const originalFieldTextApplyColor = Blockly.FieldTextInput.prototype.applyColour;

Blockly.FieldTextInput.prototype.applyColour = function () {
    originalFieldTextApplyColor.call(this);
    // @ts-ignore
    updateBorderRect(this.sourceBlock_, this.borderRect_);
};

const originalFieldNumberApplyColor = Blockly.FieldNumber.prototype.applyColour;

Blockly.FieldNumber.prototype.applyColour = function () {
    originalFieldNumberApplyColor.call(this);
    // @ts-ignore
    updateBorderRect(this.sourceBlock_, this.borderRect_);
};
maribethb commented 11 months ago

We're on board with the plan of adding whatever support we need to use css instead of themes broadly (that probably deserves its own issue if it doesn't already have one). We didn't discuss how to prioritize this feature request in light of that effort. If we're going to work on the theme/css issue soon then it probably doesn't make sense to add an option for this to the existing theme/renderer system, but we haven't prioritized when we'll work on that.

maribethb commented 10 months ago

Decision: Go ahead and add this constant since someone is asking for it now, but in the future work towards using css instead of themes.