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

Toggle Default Collapsed or Expanded #122

Merged
merged 7 commits into from
Sep 4, 2023

Conversation

minermaniac447
Copy link
Contributor

Implements #111; currently, the default setting is "expanded" (true). This could be changed to "collapsed" (false) to match current implementation.

Copy link
Owner

@tim-hub tim-hub left a comment

Choose a reason for hiding this comment

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

My suggestion is to make the Expand enable by default, without adding a new setting.

Two reasons:

  1. The purpose of this plugin is to render verses directly, the old collapsailbe (close by default) breaks this purpose, which need to be fixed (Thanks this PR)
  2. There have been too many setting toggles, we should try not add more.

src/verse/BaseVerseFormatter.ts Outdated Show resolved Hide resolved
src/ui/BibleReferenceSettingTab.ts Outdated Show resolved Hide resolved
src/data/constants.ts Outdated Show resolved Hide resolved
src/data/constants.ts Outdated Show resolved Hide resolved
@minermaniac447
Copy link
Contributor Author

  1. The purpose of this plugin is to render verses directly, the old collapsable (close by default) breaks this purpose, which need to be fixed (Thanks this PR)

I somewhat agree, with the exception of mobile users - when inserting long passages (i.e. a whole chapter for some sermon topics), it can take up the majority, if not more than that, of the screen, which can get frustrating. For most desktop users and for small verses this isn't a problem but I do think having the option to insert small is a good idea

  1. There have been too many setting toggles, we should try not add more.

I agree that we have a lot of tagging ones but it seems like there are already changes in the works to deal with this; personally I'd always rather customizability over removing options. If you're adamant on this position, could I suggest a compromise of sorting the settings into collapsible toggles?

(I'll also note I did want to make this toggle only visible when collapsible was enabled in the first place but didn't yet know how to do that; that's a change that could be implemented in a few other places too too simplify things, as part of a potential UI update pr later)

@tim-hub
Copy link
Owner

tim-hub commented Sep 4, 2023

  1. The purpose of this plugin is to render verses directly, the old collapsable (close by default) breaks this purpose, which need to be fixed (Thanks this PR)

I somewhat agree, with the exception of mobile users - when inserting long passages (i.e. a whole chapter for some sermon topics), it can take up the majority, if not more than that, of the screen, which can get frustrating. For most desktop users and for small verses this isn't a problem but I do think having the option to insert small is a good idea

  1. There have been too many setting toggles, we should try not add more.

I agree that we have a lot of tagging ones but it seems like there are already changes in the works to deal with this; personally I'd always rather customizability over removing options. If you're adamant on this position, could I suggest a compromise of sorting the settings into collapsible toggles?

(I'll also note I did want to make this toggle only visible when collapsible was enabled in the first place but didn't yet know how to do that; that's a change that could be implemented in a few other places too too simplify things, as part of a potential UI update pr later)

@minermaniac447 thanks for understanding.

Would you like to make the changes?

If so, I will merge and make it happen in next release.

@minermaniac447
Copy link
Contributor Author

Yep, I can put the change in. If you do want to revisit this idea later, let me know!

minermaniac447 and others added 4 commits September 3, 2023 22:54
Co-authored-by: Tim <[email protected]>
Co-authored-by: Tim <[email protected]>
Co-authored-by: Tim <[email protected]>
Co-authored-by: Tim <[email protected]>
@tim-hub
Copy link
Owner

tim-hub commented Sep 4, 2023

Yep, I can put the change in. If you do want to revisit this idea later, let me know!

Thanks,

@tim-hub tim-hub merged commit 91c023e into tim-hub:master Sep 4, 2023
2 checks passed
@tim-hub tim-hub mentioned this pull request Sep 4, 2023
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.

2 participants