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

Make mode placeholder value more intuitive #74

Closed
iandunn opened this issue Apr 10, 2019 · 1 comment
Closed

Make mode placeholder value more intuitive #74

iandunn opened this issue Apr 10, 2019 · 1 comment
Labels
[Component] Blocks All custom blocks (Speakers, CrowdCast, Live Schedule, etc)

Comments

@iandunn
Copy link
Member

iandunn commented Apr 10, 2019

Migrated from #33 (comment)

This feels like of unintuitive to me, like from the context I expect mode to be a boolean, but then it's name doesn't describe what it is, so I'm left confused.

I had to look through other parts of the code to realize it's a string that has an empty value until a mode like "all speakers" or "group of speakers" is selected.

I wonder if the code would be more descriptive/self-documenting/obvious if we gave mode an initial value of placeholder, and then changed things like this to read 'placeholder' !== mode ?

@iandunn iandunn added this to the Blocks v1 milestone Apr 10, 2019
@coreymckrill coreymckrill removed this from the Blocks v1 milestone Apr 11, 2019
@iandunn iandunn added [Component] Blocks All custom blocks (Speakers, CrowdCast, Live Schedule, etc) [Priority] 3 labels Jun 21, 2019
@ryelle
Copy link
Contributor

ryelle commented Aug 5, 2019

I'm going to close this issue, I don't think we gain much by making this change at this time, but we could think about this for future blocks. Feel free to reopen if you disagree.

@ryelle ryelle closed this as completed Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Component] Blocks All custom blocks (Speakers, CrowdCast, Live Schedule, etc)
Projects
None yet
Development

No branches or pull requests

3 participants