-
-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add toggle command for Bible Icon Prefix setting, and add collapsible option for callout options #251
base: master
Are you sure you want to change the base?
Conversation
Hi @AlphaHasher Thanks for your contribution, if you don't mind, can you explain the differences? From the screenshot, I cannot see the differences between with or without |
None = 'None' | ||
} | ||
|
||
export const CollapsibleVersesCollection = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I say keep it None. Also, the setting you point out does not affect the new option I added.
To make it clearer, I started testing (on my dev branch) disabling settings depending on other settings (e.g., if you don't want to use callouts, then the collapsible option can be disabled) but I ran into odd performance issues which I documented there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you take a screenshot when the setting is CollapsibleVerses.None, at the same time, reference position is Header?
I am not sure what are the differences.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I make a humble suggestion?
- To split the
changes and collapsing changes to 2 different PRs.
changes are very clear and straight-forward.- The collapsing changes I am not very sure.
Yes, I'll split them up |
I have an idea. Since this pr relates to callouts, there can be a single settings whether the user wants to use callouts or not. If yes, it will shows more options, like whether or not they want it to be collapsible or not. Right now it is complicated and the end user would not know what settings do what. |
The culprit:
The change made:
Notice the extra space that the extra
<br/>
tag makes when it is included?