-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
chore: Add Studio UI to Cypress 10 #23537
chore: Add Studio UI to Cypress 10 #23537
Conversation
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
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.
Some misc comments, nothing big -- I think the URL logic still has some bugs, I'm going to work on those in the meantime. New styles look 💯
|
||
</script> | ||
|
||
<style lang="scss"> |
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 should use one of the coolest Vue features, scoped styling.
https://vue-loader.vuejs.org/guide/scoped-css.html
Just add <style lang="scss" scoped>
and the CSS will only be applied inside this component.
border-style: solid; | ||
border-color: transparent transparent #1B1E2E transparent; | ||
} | ||
} |
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.
Assuming this is too specific to do with Tailwind?
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.
yeah, I figured this was better than adding it to the already cluttered class
attribute on the outer container. Definitely open to suggestions for how to better deal with this though
@@ -206,6 +212,8 @@ export const useStudioStore = defineStore('studioRecorder', { | |||
} | |||
} | |||
|
|||
this.setUrl(getAutUrl()) |
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.
No need to do getAutUrl
like this, we should be able to do
const autStore = useAutStore()
this.setUrl(autStore.url)
Although I think there's a bug around this anyway, as Mike pointed out in Slack, there's some weirdness around the URL logic.
@@ -77,7 +77,7 @@ const props = withDefaults(defineProps<{ | |||
}>(), { | |||
modelValue: false, | |||
helpText: `${defaultMessages.links.needHelp}`, | |||
helpLink: 'https://on.cypress.io', | |||
helpLink: '', |
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.
Any reason to make this default value ''
?
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.
Right I see - to hide the link for modals where a "Help Link" doesn't make sense.
Quick refactor to support this and to get CI green #23545
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.
Yeah, this just makes it so that if you don't pass a link then we don't show the link section in the modal header
This modal seems kind of ugly to me:
|
@astone123 I need these changes to work on the E2E tests, and it looks like we've got a few failures. I'm going to go ahead and get CI green and merge this -- we will solicit a full review by EOD tomorrow, I'm thinking since the PR is pretty big, we get specific people to do specific things - eg, someone from CT would look at the UI components, someone from E2E the tests/wiring, etc. |
* fix test * fix test * add noHelp link to StandardModal
Yeah I'm not super happy with the modal. The icon can definitely be taken out of the header. For the save button, I was just following the designs, but I agree that the check icon doesn't need to be there. @abelb what do you think based on that screenshot? ^ |
* chore: wire up Cypress Studio (#23413) * wip * wip * wip - spike * more wip [skip ci] * update style * fix ts * move types around * extract types * lint * fixing tests * fix component test * skip some tests * do not error on experimentalStudio flag * add studio controls placeholder * fixing tests * revert * revert changes * rename store * rename method * remove comment * refactor * correctly feature flag studio * simplify code * simplify code * lift check into useEventManager * correctly hide create studio prompt based on flag; * remove superfulous css * rename variables * fix bugs * wip * unskip tests * unskip more tests * fix a bug in the assertion API * fix bug in assertions [skip ci] * wip - bugs [skip ci] * feat: add experimentalStudio flag back (#23506) Co-authored-by: astone123 <adams@cypress.io> * chore: Add Studio UI to Cypress 10 (#23537) * wip * wip * wip - spike * more wip [skip ci] * update style * fix ts * move types around * extract types * lint * fixing tests * fix component test * skip some tests * do not error on experimentalStudio flag * add studio controls placeholder * fixing tests * revert * revert changes * rename store * rename method * remove comment * refactor * correctly feature flag studio * chore: wip add barebones studio modals * simplify code * simplify code * lift check into useEventManager * correctly hide create studio prompt based on flag; * remove superfulous css * chore: style studio toolbar * chore: misc feedback * chore: remove studio store prop * chore: studio URL prompt and other changes * update component * chore: UI styling and remove studio init modal * chore: revert unnecessary changes * chore: fix types * chore: fix some tests, minor refactor (#23545) * fix test * fix test * add noHelp link to StandardModal Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com> * test: studio e2e tests (#23546) * add basic e2e test * add some e2e tests for studio and a note on limitations * additional spec * add more tests, refactor helper * fix bug in studio * remove test code * chore: UI feedback * fix race condition * update tests * rename test * improve types in reporter * remove dead code * improve tests * merge tests into one spec * chore: Cap instruction modal width; exit studio mode when new spec is chosen * chore: Only render studio error when test has failed; add test for studioEnabled * correctly check if command is studio or not * improve specs and hopefully reduce flake * communicate studio state from app->reporter * receive studio save state validity from app * fix test * improve test coverage * fix external link Co-authored-by: astone123 <adams@cypress.io>
User facing changelog
This PR adds the Cypress studio UI back into the app with some styling changes. Note that this change will be merged into the
feat/studio-cypress-10
branch to be reviewed all together and merged at a later time.Steps to test
experimentalStudio: true
your project's Cypress configuration undere2e
How has the user experience changed?
PR Tasks
Deleting the PR checklist since we'll be handling more testing, documentation, etc when we merge the feature branch.