-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: move recording selector position #413
feat: move recording selector position #413
Conversation
</> | ||
)} | ||
<RecordingSelector /> | ||
<Flex align="center" justify="between" gap="2"> |
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 only change here was wrapping the content in this Flex
component to achieve responsiveness.
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.
Code looks good, added a few questions/suggestions that I'd like to hear your thoughts on
@@ -65,7 +65,10 @@ export function RecordingSelector() { | |||
id="recording-selector" | |||
placeholder="Select recording" | |||
css={css` | |||
width: 250px; | |||
width: 200px; |
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.
Now that we have more horizontal space, I'd expect it to be wider rather than narrower than before, because with the addition of the starting URL in recording names, the names became even longer.
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 was mainly trying to keep things concise on the right side of the top bar, but given that names now can be longer, it makes sense to make it wider.
/> | ||
</> | ||
)} | ||
<RecordingSelector /> |
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.
I considered using a vertical divider as an alternative, but I kept the extra margin so it's easier to manage in smaller resolutions.
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.
LGTM 🚀
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.
LGTM! 🚀
Description
This PR moves the Recording dropdown from the "Requests" tab in the Preview section to the top bar on the Generator page.
How to Test
Checklist
npm run lint
) and all checks pass.npm test
) and all tests pass.Screenshots (if appropriate):
Smaller window size:
Related PR(s)/Issue(s)
Resolves #317