Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New variant for tags-mode #78
New variant for tags-mode #78
Changes from 4 commits
11240a6
cded2de
3db95ff
995b45c
d877feb
4d66dce
08ec29f
d800757
792ee34
f9607d0
d1feae9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this is unnecessary code duplication IMO. All you need to do is change like 717 to something like:
and you have a version that works for both
:quick_tags
and non-:quick_tags
modes :)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.
this is unnecessary code duplication IMO. All you need to do is change like 746 to something like:
and you have a version that works for both
:quick_tags
and non-:quick_tags
modes :)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 prefer
@mode in [:tags, :quick_tags]
, I think it's more readable (nitpick, I know)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.
Agree!
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 love that you went out of the way to create this toggle to show how to style the options as checkboxes!
Can we please rename it to something more specific like "Options styles as checkboxes" ?
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.
Suggestion: let's remove this code and all other changes in this file related to the
selected_option_class
override, and add this:just before we create the changeset. Advantages:
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.
Yeah, what I had done was kind of a hack to just to make it work.
This is nice, but it seems to not reset when you change back to tags mode.
Maybe do a reset of
selected_option_class
in the else clause or just let it be?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 think just let it be, at least for now. I mean if the user changes the style while in quick_tags mode you don't want it to be reset when you change back to tags mode... the user will lose what they entered.
I guess for now this is the best we can do, we can come up with a better way later :)
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 love that you went out of the way to create this toggle to show how to style the options as checkboxes!
Can we please rename it to something more specific like "Options styles as checkboxes" ?
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.
the argument of the function component should also be renamed to
options_styled_as_checkboxes