Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type checking for Measurables #2946

Closed
rachel-fenichel opened this issue Aug 31, 2019 · 1 comment
Closed

Type checking for Measurables #2946

rachel-fenichel opened this issue Aug 31, 2019 · 1 comment
Assignees
Labels
component: rendering issue: bug Describes why the code or behaviour is wrong

Comments

@rachel-fenichel
Copy link
Collaborator

Right now Measurables are checked with a combination of string equality tests against the type property and calls to isCorner(), isInput(), etc.

We need to pick a standard approach and use it everywhere.

From discussion with @samelhusseini :

I would prefer that as well. I wanted to leave the types as is round corner instead of adding the left, as there's a few places that check the type directly rather than using the base method isRoundCorner.

In terms of dealing with types, here are a few options:

  • do what we're doing now, con with that is that the check method is on the measurable, ie: to add additional types you have to do them in base measurable, otherwise elem.isSpecialType() might not exist on all elements.
  • factory type checker, something like: Blockly.isSpecialType(elem) that gets passed in an element and checks the type. This can be extended independently in each renderer to types that only matter there.
  • Use a enum flag and bitwise operations with types. This is the option I prefer, but it might be an overkill. We could have a central place to register types, which basically internally appends new types to a enum flag. Then you can do things like:
    if (elem.type & Blockly.Measurable.Types['left row']) {
    }
    The nice thing about enum flags is you can have union types, so you can do bitwise checks to find if an element is a corner (not really caring about the specific type of corner it is), but then again you could always just do indexOf('corner') haha.

A combination of 2 and 3 would be nice too.

@rachel-fenichel rachel-fenichel added issue: bug Describes why the code or behaviour is wrong issue: triage Issues awaiting triage by a Blockly team member labels Aug 31, 2019
@rachel-fenichel rachel-fenichel removed the issue: triage Issues awaiting triage by a Blockly team member label Aug 31, 2019
@samelhusseini samelhusseini self-assigned this Sep 3, 2019
@samelhusseini
Copy link
Contributor

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: rendering issue: bug Describes why the code or behaviour is wrong
Projects
None yet
Development

No branches or pull requests

2 participants