-
Notifications
You must be signed in to change notification settings - Fork 351
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
Make commands editable and re-usable #1268
Conversation
@@ -452,14 +452,14 @@ const switchToRequestedDb = (store: any) => { | |||
const activeConnection = getActiveConnectionData(store.getState()) | |||
const requestedUseDb = activeConnection?.requestedUseDb | |||
|
|||
const useDefaultDb = () => { | |||
const switchToDefaultDb = () => { |
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.
renamed because of react hooks lint rule
export const REQUEST_SENT = 'requests/SENT' | ||
export const CANCEL_REQUEST = 'requests/CANCEL' | ||
export const REQUEST_CANCELED = 'requests/CANCELED' | ||
export const REQUEST_UPDATED = 'requests/UPDATED' |
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.
Typescript struggles to correctly type the actions if they´re not simple strings
getSettings, | ||
REPLACE, | ||
UPDATE | ||
getSettings |
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 few unused imports
this.code = code | ||
// @ts-expect-error ts-migrate(2683) FIXME: 'this' implicitly has type 'any' because it does n... Remove this comment to see the full error message | ||
this.message = message | ||
class UseDbError extends Error { |
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.
Threw together an a better Error to avoid ts-errors
if (response) { | ||
if (response.then) { | ||
response.then((res: any) => { | ||
if (res) { |
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.
Moved the conditions in a way I found easier to read
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.
Wow, I like this feature more than I thought!
The user experience is actually really good, it's no big deal to just click in a query in the stream and edit it.
Well done!
I've left some comments but the main issue is around fullscreen 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.
8bd4473
to
63f9702
Compare
I've gone through your comments, tests pass locally and the preview is updated (http://reusable-frame.surge.sh/). Awaiting more feedback/green pipe :) |
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.
Not all frames seems to have the "exit fullscreen" button, which would be needed now that all frame can go into fullscreen.
Example:
- On
:play start
that shows up, click the frame editor. - Press Esc to go into fullscreen on that frame
- Click on an input field on the frame
- Press Esc.
- Nothing happens and there's no obvious way for the user how to get out of the fullscreen state (click editor + press Esc is the only way out)
Oh right forgot about that, I've removed the conditional render of it now! @oskarhane |
const cmd = frame.cmd.replace(/^:/, '') | ||
const Frame = cmd[0].toUpperCase() + cmd.slice(1) + 'Frame' | ||
MyFrame = require('./Extras/index')[Frame] | ||
if (!MyFrame) { | ||
MyFrame = getFrame(frame.type) |
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 was written for us to sneak in fun frames without having to explicitly import them. I think we should keep it :)
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.
It's always easier to take fun out than put it in, I'll keep it as is then :)
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.
Executing:
RETURN 1;
RETURN "hej";
kills the stream (at least when I'm not connected to a DBMS)
}, | ||
[reusableFrame]: { | ||
name: reusableFrame, | ||
on: true, | ||
displayName: 'Enable reuseable frame', | ||
tooltip: 'Edit and rerun right in a frame' |
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.
👍
const rollDownAnimation = keyframes` | ||
from { | ||
transform: translate(0, -${dim.frameBodyHeight}px); | ||
max-height: 0; | ||
} | ||
to { | ||
transform: translateY(0); | ||
max-height: 500px; /* Greater than a frame can be */ | ||
} | ||
` | ||
export const AnimationContainer = styled.div` | ||
animation: ${rollDownAnimation} 0.4s ease-in; | ||
` |
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.
👍
Good that you caught the multi statement issue! Should be resolved now. |
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.
Let's do this 🎞️
This pull request aims to make it possible to edit and re-run frames. It's added as an experimental setting for now, but defaults to on for ease of testing.
I've annotated the source code to explain what's changed and why. Apologies for the size of the PR, I recommend setting the "side by side" diff ignoring whitespace to get a better view of the bigger changes.
Demo: https://user-images.githubusercontent.com/10564538/105364088-098cad80-5bfd-11eb-9869-d606b59699a6.mp4
Try it yourself at: http://reusable-frame.surge.sh/