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.

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

Add ability to zoom in / out in Electron browser #4728

Merged
merged 9 commits into from
Jul 18, 2019
Merged

Conversation

jennifer-shehane
Copy link
Member

@jennifer-shehane jennifer-shehane commented Jul 16, 2019

Can zoom this

Screen Shot 2019-07-16 at 4 01 49 PM

Can also zoom this in Electron browser

Screen Shot 2019-07-16 at 4 00 14 PM

@jennifer-shehane jennifer-shehane requested a review from a team July 16, 2019 09:32
Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

There's a failing test in /root/cypress/packages/server/test/unit/gui/menu_spec.coffee

@jennifer-shehane jennifer-shehane requested review from flotwig and a team July 17, 2019 06:51
chrisbreiding
chrisbreiding previously approved these changes Jul 17, 2019
@brian-mann brian-mann self-requested a review July 17, 2019 16:25
Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

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

When you are decaffeinating files - you must decaffeinate the files first AND submit a PR for that separately.

As it stands - there is no easy way to tell the actual "diff" between the changes you made to menu.coffee and menu.js or menu_spec.coffee and menu_spec.js since menu.coffee and menu_spec.coffee were first deleted.

To be clear - you don't have to submit these PR's serially. You can go ahead and decaffeinate locally first - and continue working as normal. But once you're "finished" you need to then open another PR (off of develop) and decaffeinate the files you ended up decaffeinating.

This second PR can then be merged, and then the first PR will correctly show the diff against the decaffeinated files in develop. Ask @bkucera if you have any questions since he does this on a regular basis. I will do this for you this time.

@jennifer-shehane
Copy link
Member Author

jennifer-shehane commented Jul 18, 2019

@brian-mann This should be clearly documented somewhere - as I thought there was a process in line, but never was told it, but from what I can tell you:

  1. Rename the file from .coffee to .js and commit (do not decaffeinate yet).
  2. Decaffeinate the .js file and commit.
  3. Clean up any issues / eslinting of new .js file and commit.
  4. Now make any changes you wanted to make from the original issue/PR you were trying to address.

@jennifer-shehane jennifer-shehane dismissed stale reviews from brian-mann and flotwig July 18, 2019 06:34

decaffeinate was done in other PR for menus

@jennifer-shehane
Copy link
Member Author

The only failure is the one caused by cypress.io - which is known, so merging in with this failure.

@jennifer-shehane jennifer-shehane merged commit e06ad78 into develop Jul 18, 2019
@jennifer-shehane jennifer-shehane deleted the issue-1231 branch July 18, 2019 07:56
@brian-mann
Copy link
Member

brian-mann commented Jul 18, 2019

Explaining the decaffeinate process was explained at the time that it mattered, during PR / peer review.

Once you figure out that you're going to decaffeinate stuff, you extract that part out of your PR and open a separate PR prior to merging the original. It's the best time to do it, so it's only done on the files that are affected by the PR. They need to be merged ahead of time so that the diff is accurate.

I took the files you worked in and decaffeinated them and then added them to develop - so that your PR's diff could be generated off of develop, to accurately portray the changes.

@brian-mann brian-mann restored the issue-1231 branch July 18, 2019 16:42
@emilyrohrbough emilyrohrbough deleted the issue-1231 branch August 1, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement zoom in Test Runner
4 participants