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
Custom Prompt for changing options #243
Custom Prompt for changing options #243
Changes from 24 commits
e4eed2e
a229ba9
964974c
e456035
49e51de
54cbe3f
d0d4ada
ec981ac
ebaa018
79acf6c
22c5ea5
34a4e6b
b97a86f
db8d946
98c00f7
5cee331
834f867
6b147b0
5418ef7
f910593
36317c9
0eca303
580caef
e43c01d
002081b
355f611
b2c2098
664be51
52a4608
a49817f
65eaaec
1cd4f53
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.
Idea of feature for the prompt plugin: the ability to have a placeholder (would be better suited for
example
instead of putting it as value and checking later thatvalue !== example
)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 is already a placeholder which is the
value
fieldAre you talking about implementing this exact logic in
custom-electron-prompt
?I could make an boolean option
placeholder
which will mean "return null ifoutput=options.value
"but it seems kinda unnecessary..
I think it might be best to let whoever implements the prompt decide what to do with the output, since there could be many different intentions when using the prompt,
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.
Yes, idea was to add a implement the placeholder logic/field in custom-electron-prompt! (main pro of a placeholder is to avoid having a fake value to remove when filling the field, contrary to a default value - here, it feels we have a placeholder used a default value 🙃)
Not a big deal though, does not block merging - just an improvement idea for the custom-prompt package :)
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.
355f611 changed the input field "placeholder" attribute to the previous example as you suggested
(didn't need to change custom-electron-prompt itself)
I'm not sure its better though, since you can't copy paste or edit that text, and it disappears when typing. which can makes it harder for people that don't know the format
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 looks like a one-shot operation that can be done with a store migration (there are examples in in
config/store.js
)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 moved the code to store migration in 002081b
Is it alright this way? should the version be higher?
(
prompt-custom-titlebar.js
was already moved to providers folder before 😝)