feat: make isMutator public #6316
Conversation
| /** Is this workspace the surface for a flyout? */ | ||
| isFlyout = false; | ||
| get isFlyout(): boolean { |
There was a problem hiding this comment.
getters and setters are banned by the js style guide (we use them in limited cases involving moving/deprecating properties but shouldn't use them in regular code)
also i don't like having "internal" as part of the name of the property. so i would suggest flyout for the property and IsFlyout for the function, maybe? i think that would be the recommended pattern in java at least.
but i guess that would be a breaking change since isFlyout is public... so either isFlyout and getIsFlyout or something better you come up with or just scrap the idea and keep the properties only. what do you think?
There was a problem hiding this comment.
getters and setters are banned by the js style guide
They are not banned by the ts style guide though.
but i guess that would be a breaking change since isFlyout is public... so either isFlyout and getIsFlyout
Yeah this is the problem I ran into, which is why I ended up going with the getters. Because getIsFlyout() is.... not great.
My vote is either go with the getters as they currently are, or just keep them as properties only.
There was a problem hiding this comment.
Ok thanks for the pointer to the right place in the TS style guide. They are still "not recommended" because closure compiler doesn't deal with them well but this seems to fit when they're a good choice, and for future uses let's try to avoid with better property names for new properties
The basics
npm run formatandnpm run lintThe details
Resolves
N/A
Proposed Changes
Make isMutator public.
Behavior Before Change
There was no way for external developers to tell if a workspace was a mutator workspace.
Behavior After Change
There is a way for external developers to tell if a workspace is a mutator workspace.
Reason for Changes
Fixing google/blockly-samples#1169, and also just general usefulness
Test Coverage
N/A
Documentation
N/A
Additional Information
N/A