Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(unify): Remove 'Reconfigure' dropdown from Testing Type chooser #21063

Merged
merged 11 commits into from
Apr 19, 2022
Merged

fix(unify): Remove 'Reconfigure' dropdown from Testing Type chooser #21063

merged 11 commits into from
Apr 19, 2022

Conversation

davidmunechika
Copy link
Contributor

@davidmunechika davidmunechika commented Apr 13, 2022 •
edited
Loading

User facing changelog

Remove the ‘Reconfigure’ dropdown from testing type chooser.
Update: this also removes the 'Choose a Browser' dropdown from testing type chooser since the functionality of this doesn't work as intended.

Additional details

Previous version:

Screenshot 2022-04-13 at 10 49 35 AM

Updated version:

Screenshot 2022-04-13 at 10 42 55 AM

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [n/a] Has a PR for user-facing changes been opened in cypress-documentation?
  • [n/a] Have API changes been updated in the type definitions?
  • [n/a] Have new configuration options been added to the cypress.schema.json?

@davidmunechika davidmunechika self-assigned this Apr 13, 2022
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Apr 13, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Apr 13, 2022 •
edited
Loading


Test summary

4343 • 0 • 48 • 0 • Flakiness 2

Run details
Project cypress
Status Passed
Commit 9af6fdc
Started Apr 18, 2022 5:19 PM
Ended Apr 18, 2022 5:33 PM
Duration 13:49 💡
OS Linux Debian - 10.10
Browser Electron 94

View run in Cypress Dashboard ➡️

Flakiness

files.cy.js 1 Flakiness
1 ... > has implicit existence assertion, retries and throws a specific error when file does not exist for null encoding
net_stubbing.cy.ts 1 Flakiness
1 network stubbing > intercepting request > can delay and throttle a StaticResponse

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

@davidmunechika davidmunechika marked this pull request as ready for review April 13, 2022 15:35
@jennifer-shehane
Copy link
Member

@davidmunechika It looks like this percy screenshot is still showing a dropdown arrow on the E2E side. https://percy.io/cypress-io/cypress/builds/17329531/changed/972768022?browser=chrome&browser_ids=23&subcategories=unreviewed%2Cchanges_requested&viewLayout=side-by-side&viewMode=new&width=800&widths=800%2C1000

@ryanthemanuel
Copy link
Collaborator

This is maybe out of scope for this change, but I was playing around with the state of things now and the drop down we get when we're selecting a testing type from the app is a little weird.

If you click the drop down you get an option to select 'Choose a Browser'

image

However, if you click that, it doesn't take you to the "Choose a Browser" screen, it launches the testing type you selected with the same browser.

@davidmunechika
Copy link
Contributor Author

davidmunechika commented Apr 14, 2022 •
edited
Loading

@ryanthemanuel Hmm... should I just remove the dropdown altogether?

lmiller1990
lmiller1990 reviewed Apr 14, 2022
View reviewed changes
Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can greatly simplify this, now we do not have a list, but a single item (effectively a boolean... if (!props.isRunning) { /* thing */ }.

packages/frontend-shared/src/gql-components/StatusBadge.vue Outdated Show resolved Hide resolved
@davidmunechika
Copy link
Contributor Author

davidmunechika commented Apr 14, 2022 •
edited
Loading

@lachlan Don't we still want the dropdown to be controllable by the MenuButton though? So I think we need to keep the MenuItem. We don't necessarily want to show the MenuItems if !props.isRunning (or similarly if menuItems.length > 0), we just want to show the dropdown button which can expand the MenuItems. So I'm not sure we can just do a boolean here...

@davidmunechika @ryanthemanuel
use length of menuItems
8a05e5a 
Co-authored-by: Ryan Manuel <ryanm@cypress.io>
@ZachJW34
Copy link
Contributor

@davidmunechika I can't find the mocks in Figma for this dropdown, and seeing that it doesn't work as intended I say we remove it from the TestingTypeSwitcher. Would be good to sync with @ryanjwilke, but for this PR we can remove the menu and you can create a followup issue for adding back in the Choose a Browser menu item if it's still desired.

@lmiller1990
Copy link
Contributor

@davidmunechika I see you raised this internally and clarified the goal. Is this PR ready for re-review?

@davidmunechika
Copy link
Contributor Author

@lmiller1990 Yes, sorry- was waiting for the build to finish and forgot to re-request reviewers

@ryanjwilke
Copy link
Contributor

@davidmunechika We can safely remove the "Choose a browser" action and the entire menu in this PR. That should be fine. This is not something we'll need to add back in.

@ZachJW34
Copy link
Contributor

@davidmunechika can you create an issue for cleaning up the backend code related to reconfigure project?

@davidmunechika
Copy link
Contributor Author

@ZachJW34 Created one here: https://cypress-io.atlassian.net/browse/UNIFY-1593

@davidmunechika davidmunechika merged commit 691234c into 10.0-release Apr 19, 2022
@davidmunechika davidmunechika deleted the unify-1533-remove-reconfigure-dropdown branch April 19, 2022 15:56
tgriesser added a commit that referenced this pull request Apr 20, 2022
@tgriesser
Merge branch '10.0-release' into tgriesser/chore/UNIFY-1256-auto-merg…
2c8a7b3 
…e-config

* 10.0-release:
  chore: Move component-index generation to scaffold-config package (#21090)
  fix: label text should be clickable to toggle snapshot highlight (#21122)
  feat: add next preset to webpack-dev-server-fresh (#21069)
  chore: add dev-servers as deps to server to be included in the binary (#21142)
  fix: do not highlight preExtension if selected option is renameFolder (#21121)
  fix(unify): Remove 'Reconfigure' dropdown from Testing Type chooser (#21063)
  feat(unify): launchpad header breadcrumbs and reusable tooltip component (#20648)
  test: add windows-test-kitchensink job (#20847)
  fix: aut centering and height calculation (#21019)
  chore: fix tests that fail on windows (#21055)
  fix: running a new test after already having run tests (#21087)
  fix: ct project setup framework keys for next and nuxt (#21116)
  fix: remove MountReturn from scaffolded ct support file (#21119)
  fix: do not scaffold fixtures if folder exist (#21078)
  fix: revert "fix: types for Cypress.Commands.add (#20376)" (#21104)
  chore: Update Chrome (stable) to 100.0.4896.127 and Chrome (beta) to 101.0.4951.34 (#21083)
  fix: types for Cypress.Commands.add (#20376) (#20377)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZachJW34 ZachJW34 ZachJW34 approved these changes

@ryanthemanuel ryanthemanuel ryanthemanuel approved these changes

@lmiller1990 lmiller1990 Awaiting requested review from lmiller1990

Assignees

@davidmunechika davidmunechika

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@davidmunechika @jennifer-shehane @ryanthemanuel @ZachJW34 @lmiller1990 @ryanjwilke