fix: create more comprehensive field row labels #9458
Conversation
microbit-robert
requested review from
BenHenning
and removed request for
a team
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Thanks @microbit-robert! I have a couple of comments but I also want to follow up on the issue itself.
| if (!fieldRowLabel) { | ||
| const inputs = this.getSourceBlock().inputList; | ||
| const index = inputs.indexOf(this); | ||
| if (index > 0) { |
There was a problem hiding this comment.
This seems necessary to keep (at least, slice will probably misbehave without this check). That being said, I'm not sure it's even possible for this scenario to logically happen in practice, but it's definitely possible just from an API perspective.
There was a problem hiding this comment.
[1, 2, 3].slice(0, 0) returns [], so this should do no harm, but I'm happy to keep the check in.
| }); | ||
| return fields; | ||
| }) | ||
| .filter(Boolean) |
There was a problem hiding this comment.
I've actually never seen this syntax 'hack'. Is it effectively the same as .filter((value) => !!value)? Which, I suppose, is simply filtering out empty and null/undefined entries.
There was a problem hiding this comment.
Yes, this is exactly what it does in terms of API and in practice.
2 participants
The basics
The details
Resolves
Fixes #9457
Proposed Changes
This PR uses all preceding inputs to create a more comprehensive field row label.
There is a scenario where this fix won't do exactly the right thing. In the case where a block has multiple branches (e.g., an
ifblock) and also doesn't have inline text for the statement input (e.g., there is no "do"), then the entire block to the current input index is going to be output, which might be something like "Begin if true then else if false...". Thoughts on this scenario are welcome. In such situations,getFieldRowLabelcould be overridden by the developer, but I wonder if a better default can be provided.