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

Does HomeScreen need a screen name? #629

Closed
chrisklus opened this issue Apr 15, 2020 · 2 comments
Closed

Does HomeScreen need a screen name? #629

chrisklus opened this issue Apr 15, 2020 · 2 comments
Assignees

Comments

@chrisklus
Copy link
Contributor

From #627, screen.nameProperty is going to be uninstrumented for the HomeScreen (and possibly single screen sims since their screen names are null).

It feels cleanest to do this by checking if options.name exists for the screen, but HomeScreen does get passed a name string on line 41 of HomeScreen.js. While screen.nameProperty is used for labeling the buttons on the nav bar for sim screens (in NavigationBarScreenButton), the home screen's button uses its own type (HomeButton), and does not appear to use the string on the nav bar or the home screen itself. I'm wondering if it's used somewhere for interactive descriptions, but if not can it be omitted from HomeScreen.js? Thanks!

@chrisklus chrisklus changed the title Does HomeScreen need a name? Does HomeScreen need a screen name? Apr 15, 2020
@zepumph
Copy link
Member

zepumph commented Apr 15, 2020

Yes, it is used as the header for that screen:

image

I'm not sure I understand all the nuance in trying to omit that, but I feel like using value === null as a check on whether or not to instrument is a bit obfuscated. I'd be happy to discuss further if you would like.

@chrisklus chrisklus assigned chrisklus and unassigned jessegreenberg and zepumph Apr 15, 2020
@chrisklus
Copy link
Contributor Author

@zepumph and I just chatted on zoom - some discussion points from the call:

I'm not sure I understand all the nuance in trying to omit that, but I feel like using value === null as a check on whether or not to instrument is a bit obfuscated.

This could have worked out because the designers' request was to essentially not instrument this Property if the name was not visible in the sim (for screens), which is a pattern followed by every screen except the home screen. However, as @zepumph pointed out above, HomeScreen.nameProperty is appropriately being used for accessibility, but regardless, felt that using a null check is not great because a similar pattern to the HomeScreen's use case could arise for single screen sims (and consequently further break the pattern described above).

Therefore, @zepumph recommended creating an option like instrumentNameProperty that defaults to true and only not instrument nameProperty if options.name is null or instrumentNameProperty is false. Thanks! Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants