Skip to content

Fix pipeline #3122

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
Merged
merged 2 commits into from
Dec 4, 2019
Merged

Fix pipeline #3122

merged 2 commits into from
Dec 4, 2019

Conversation

@MichaelDeBoey
Copy link
Contributor

@MichaelDeBoey MichaelDeBoey commented Dec 4, 2019

CompuIves
CompuIves reviewed Dec 4, 2019
View reviewed changes
Copy link
Member

@CompuIves CompuIves left a comment

Choose a reason for hiding this comment

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

Whoah thanks for the quick fix!! This is very clearn, I have 2 questions about the design change and then it's ready to go.

packages/common/src/themes/codesandbox-black.js
editor: {
background: tokens.grays[700],
foreground: tokens.white,
foreground: tokens.grays[300],
Copy link
Member

@CompuIves CompuIves Dec 4, 2019

Choose a reason for hiding this comment

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

Should this be white again?

Copy link
Contributor Author

@MichaelDeBoey MichaelDeBoey Dec 4, 2019

Choose a reason for hiding this comment

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

I changed these values, because of your changes in #3008

Copy link
Contributor Author

@MichaelDeBoey MichaelDeBoey Dec 4, 2019

Choose a reason for hiding this comment

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

You changed these in codesandbox-black.json, but that file will be deleted and I changed the values in codesandbox-black.js which is really used

Copy link
Member

@CompuIves CompuIves Dec 4, 2019

Choose a reason for hiding this comment

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

Oh nice!! Really nice.

packages/common/src/themes/codesandbox-black.js
hoverBackground: tokens.green,
border: tokens.grays[500],
foreground: tokens.white,
foreground: tokens.grays[300],
Copy link
Member

@CompuIves CompuIves Dec 4, 2019

Choose a reason for hiding this comment

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

Should this be white again?

Copy link
Contributor Author

@MichaelDeBoey MichaelDeBoey Dec 4, 2019

Choose a reason for hiding this comment

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

I changed these values, because of your changes in #3008

@MichaelDeBoey
Copy link
Contributor Author

MichaelDeBoey commented Dec 4, 2019

Since snapshot test failed, I also updated the snapshot

@MichaelDeBoey
Copy link
Contributor Author

MichaelDeBoey commented Dec 4, 2019

@CompuIves All green, only drone needs to run now 🙂
Can't cancel all of my other PRs' drone runs, so maybe you could cancel them? 🤔

@lbogdan
Copy link
Contributor

lbogdan commented Dec 4, 2019 •
edited
Loading

Build for latest commit bf09a93 is at https://pr3122.build.csb.dev/s/new.

@CompuIves CompuIves merged commit 20063f5 into codesandbox:master Dec 4, 2019
@MichaelDeBoey MichaelDeBoey deleted the fix-pipeline branch December 4, 2019 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CompuIves CompuIves CompuIves left review comments

Assignees

No one assigned

Labels

🚨 ASAP 🏗️ CI/CD

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MichaelDeBoey @lbogdan @CompuIves