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
streamer mode fix #512
base: main
Are you sure you want to change the base?
streamer mode fix #512
Changes from 3 commits
56d8bfd
097843d
571f281
36133c2
d748a26
8732317
c908073
96435f7
67178af
fa5632a
4ba8f06
09dcde8
8b4fe4c
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 feels weird to put it in this component. Instead, this dialog should be it’s own code that is called from the “Secrets” button. When user confirms, it will continue to open the editor.
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.
That could be an approach, but this will work for when I've implemented routing the secrets page will have it;s own url
/secrets
the button will be a link to the page not a change in state...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.
We can always attach an event listener to the link to avoid it linking in streamer mode.
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 don't understand why you're not ok with this being in the editor.
There was already logic for the editor to handle secrets differently (no install & ace websocket not started)
so I don't see why it cannot be given 1 extra parameter (streamer mode) in order to show the warning 1st
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.
A component should do 1 thing. This makes it very complicated with doing completely different things. Also, it's a prompt to a user so we should follow the convention of asking that as a dialog.
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.
OK created a dialog in
streamer-warning.ts
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.
@balloob are you happy with this change?