feat: Recording selector draft #36
Conversation
going-confetti
commented
Jul 16, 2024
•
going-confetti
commented
| export function useHasRecording() { | ||
| return useGeneratorStore((state) => state.requests.length > 0) | ||
| } |
There was a problem hiding this comment.
@e-fisher what's your favorite way of defining reusable selectors? Maybe the should be callbacks that we then import and pass to useGeneratorStore?
There was a problem hiding this comment.
would this approach work well ? Seems like a clean way to have a file defining selectors, importing them and using with the useGeneratorStore
There was a problem hiding this comment.
Yes! That's what I meant by callbacks that we then import and pass to useGeneratorStore, I should've been more specific
There was a problem hiding this comment.
I'd probably vote for the callback approach just because it's a bit more explicit -
useHasRecording - hook named this way could be doing anything
useGeneratorStore(selectHasRecording) - indicates that value is read from store
| {requests.length > 0 && ( | ||
| <Button onClick={() => setShowAllowListDialog(true)}> | ||
| Allowed hosts [{allowList.length}/{hosts.length}] | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
Not needed until we have the recording
| <Button onClick={saveGenerator}>Save</Button> | ||
| <Button onClick={handleImport}>Import HAR</Button> | ||
| <RecordingSelector /> | ||
| <AllowList /> | ||
| <Button onClick={handleExport} disabled={!hasRecording}> | ||
| Export script | ||
| </Button> | ||
| {hasRecording && <Button onClick={saveGenerator}>Save</Button>} | ||
| {hasRecording && <Button onClick={handleExport}>Export script</Button>} |
There was a problem hiding this comment.
Took the liberty of rearranging the buttons to have the most important ones on the right + hiding save, export, and allow list until there's an actual recording.
It still looks a bit crowded, but probably the biggest UX issue is that primary buttons are overused in the header. We'll probably tackle this once the design is ready
| @@ -64,7 +51,7 @@ export function Generator() { | |||
| </Allotment.Pane> | |||
| </Allotment> | |||
| </Allotment.Pane> | |||
| <Allotment.Pane minSize={300}> | |||
| <Allotment.Pane minSize={300} visible={hasRecording}> | |||
There was a problem hiding this comment.
Similar reasoning as with the header buttons: no need to show the sidebar if it's empty
| <CaretDownIcon /> | ||
| </IconButton> | ||
| </Popover.Trigger> | ||
| <Popover.Content size="1" align="end"> |
There was a problem hiding this comment.
This will have a scrollable/searchable list of saved recordings once we implement filesystem integration and all that
| <Flex gap="2"> | ||
| <Popover.Close> | ||
| <Button variant="outline" asChild> | ||
| <Link to="/recorder">Start recording</Link> |
There was a problem hiding this comment.
Currently, just redirects to the recorder page, but should also start a new recording in future
| export function useHasRecording() { | ||
| return useGeneratorStore((state) => state.requests.length > 0) | ||
| } |
There was a problem hiding this comment.
would this approach work well ? Seems like a clean way to have a file defining selectors, importing them and using with the useGeneratorStore
| @@ -58,3 +59,8 @@ export const useGeneratorStore = create<GeneratorState>()( | |||
| }), | |||
| })) | |||
| ) | |||
|
|
|||
| function generateNewName() { | |||
| const date = new Date().toISOString().split('T')[0] | |||
There was a problem hiding this comment.
I wouldn't split it, so that if you are working on multiple ones you can save them with a different name on the same day 🤔
There was a problem hiding this comment.
Oops, you're right! A fix is incoming in a bit 👀
There was a problem hiding this comment.
Changed it, but used the US format 🤔. We should probably use the same format we use in GCk6 for test names
There was a problem hiding this comment.
I think format is a 🔥 topic, using US format would add unneeded confusion 🤔
Maybe the ideal is to use the common syntax of YYYY-MM-DD ? What do you think ?
You could argue that the format you are using is more readable, so I'm not completely sure. I would still slightly prefer the format that is readable out of the box for both US & Europe, but maybe we can gather more feedback.
Remembering that this is just the generator file name, I don't think we have a strong need for keeping the format used in test names 🤔
There was a problem hiding this comment.
using US format would add unneeded confusion 🤔
That's fair
Maybe the ideal is to use the common syntax of YYYY-MM-DD ? What do you think ?
I'd rather use a format we use in other parts of k6 offerings, for consistency reasons
Remembering that this is just the generator file name, I don't think we have a strong need for keeping the format used in test names 🤔
As of now, it's not actually a part of the filename, but I did think about adding it to the filename
There was a problem hiding this comment.
@Llandy3d I don't have a strong opinion actually, let's discuss it some time next week and pick one format 👍
| @@ -0,0 +1,53 @@ | |||
| import { useGeneratorStore, useHasRecording } from '@/hooks/useGeneratorStore' | |||
There was a problem hiding this comment.
Some unused imports in this file, maybe we could add this plugin to clear imports automatically (we already run eslint --fix on commit)
https://www.npmjs.com/package/eslint-plugin-unused-imports
