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

Fix label ordering #22388

Merged
merged 9 commits into from May 15, 2018
Merged

Fix label ordering #22388

merged 9 commits into from May 15, 2018

Conversation

balderdash
Copy link
Contributor

@balderdash balderdash commented May 14, 2018

Trailing labels aren't being rendered correctly under the current custom block parser, e.g.:

  {
    "func": "blahblah",
    "blockText": "before input {INPUT} after input",
    "args": [
      {
        "name": "INPUT"
      }
    ]
  }

Before:
image
After:
image

Things get really weird with multi-line blocks:

  {
    "func": "dosomething",
    "blockText": "val 1 {VALUE1} field input {FIELD} val 2 {VALUE2} end",
    "args": [
      {
        "name": "FIELD",
        "field": true
      },
      {
        "name": "VALUE1"
      },
      {
        "name": "VALUE2"
      }
    ],
    "inline": false
  }

Before:
image

After:
image

The code change to make this happen was grouping inputs into inputRows and handling each one independently (replacing the broken and ugly getPreviousInput approach I had tried earlier). Each inputRow is just a group of inputs ending with a line-ending input, i.e. a value, statement, or dummy input. The labels and field inputs in each row can now be assembled in a fairly straightforward way:

inputRows.forEach(inputRow => {
// Create the last input in the row first
const lastInputConfig = inputRow[inputRow.length - 1];
const lastInput = inputTypes[lastInputConfig.mode]
.addInputRow(blockly, block, lastInputConfig);
// Append the rest of the inputs onto that
inputRow.slice(0, -1).forEach(inputConfig => {
inputTypes[inputConfig.mode].addInput(blockly, block, inputConfig, lastInput);
});
// Finally append the last input's label
lastInput.appendTitle(lastInputConfig.label);
});

For inline blocks, the rows all just string together in a long line, looking just like blockText (but a block),

The other bit of refactoring that made sense to do here was adopting the customInputTypes config format for all the input types (this is the thing I added for the location picker input in #21955).

Ram Kandasamy added 6 commits May 13, 2018 22:37
I liked the way I set up app-sepcific input types so much I converted
all of the standard inputs the same way.
@balderdash balderdash requested a review from Hamms May 14, 2018 18:39
Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

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

This is great!

I'd love to see some tests

* @param {object} customInputTypes A map of customType input names to their
* definitions, which are objects that have `addInput` and `generateCode`
* @param {object} inputTypes A map of input types to their definitions, which
* are objects that have `addInput` and `generateCode`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could we get a jsdoc @typedef for input definitions?

}
});
const lastRow = inputRows[inputRows.length - 1];
if (inputRows[inputRows.length - 1].length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could just be lastRow.length

return Blockly.JavaScript.valueToCode(this, arg.name, ORDER_COMMA);
}
const inputConfig = inputConfigs.find(input => input.name === arg.name);
return inputTypes[inputConfig.mode].generateCode(this, inputConfig);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@balderdash
Copy link
Contributor Author

balderdash commented May 15, 2018

There are some (clearly insufficient) unit tests in blockUtilsTest.js, but I'd like to add some kind of block screenshot tests that actually verify that things are rendering the way we want them to.

@balderdash balderdash merged commit 42d9db1 into staging May 15, 2018
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.

None yet

2 participants