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

Update app.rst #2297

Merged
merged 1 commit into from Nov 21, 2022
Merged

Update app.rst #2297

merged 1 commit into from Nov 21, 2022

Conversation

oguz-hanoglu
Copy link
Contributor

What changes are proposed in this pull request?

The explanation for 'all' option sidebar mode was not clear probably due to a typo.

The explanation for 'all' option sidebar mode was not clear probably due to a typo.
@@ -302,8 +302,7 @@ samples or fields, this may involve substantial computation.

Therefore, the App supports three sidebar modes that you can choose between:

- `all`: always compute counts for all fields and stats for all fields whose
filter tray is expanded
- `all`: always compute counts for all fields and stats
Copy link
Contributor

@brimoor brimoor Nov 13, 2022

Choose a reason for hiding this comment

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

What the previous version of the docs were trying to convey is that, in all mode:

  • The count values will be shown for all fields that are visible in the sidebar
  • In addition, the data inside the "filter tray" (eg the id and label widgets you see after clicking the down caret in the image below) will be computed whenever the sidebar loads

Do you have any suggestion for how to edit the docs in a different way to make that more clear? In your proposed version I think it may not be clear what "stats" means without some additional context

Screen Shot 2022-11-13 at 1 58 22 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Thanks for the detailed clarification.

After your explanation, I agree that my proposed version needs updating.

As an outsider reading the document(before your explanation), I did not know what "stats" is. Referring to it as 'stats' may be confusing when there is also a 'Statistics tabs' in the App.

My confusion led me think that there is a typo considering the redundant for all fields in 'all' bullet.

So, my new suggestion is:

  • all: always compute counts (for all fields -> this part can be removed) and stats (-> this can be renamed) for all fields whose filter tray is expanded

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good 👍

Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

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

Thanks for your help on improving these docs! I'll add a quick edit to resolve the outstanding thread after merging

@brimoor brimoor merged commit 17a30d3 into voxel51:develop Nov 21, 2022
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