Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Changed K+ to K?#4273

Closed
Subtlesnow wants to merge 3 commits intofirefox-devtools:masterfrom
Subtlesnow:master
Closed

Changed K+ to K?#4273
Subtlesnow wants to merge 3 commits intofirefox-devtools:masterfrom
Subtlesnow:master

Conversation

@Subtlesnow
Copy link

@Subtlesnow Subtlesnow commented Oct 4, 2017

Associated Issue: #4213

Here's the Pull Request Doc
https://devtools-html.github.io/debugger.html/CONTRIBUTING.html#pull-requests

Summary of Changes

  • Took out SVG component that was for K+ and added K? in text because we couldn't find another SVG icon.
  • Moved the button to the right.
  • Soften Color to match accordion

Copy link
Contributor

@codehag codehag left a comment

Choose a reason for hiding this comment

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

thanks for the pr! a couple of issues, lets discuss :)

renderVerticalLayout: Function;
toggleSymbolModal: Function;
onEscape: Function;
onCommandSlash: Function;
Copy link
Contributor

@codehag codehag Oct 4, 2017

Choose a reason for hiding this comment

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

we have this work done in https://github.com/devtools-html/debugger.html/pull/4265/files. so lets remove these changes from this PR :)

flex: 0 0 29px;
border-bottom: 1px solid var(--theme-splitter-color);
display: flex;
justify-content: flex-end;
Copy link
Contributor

Choose a reason for hiding this comment

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

this also moves all of the stepping buttons over to the right hand side of the bar, which is not something we want to do right now. lets remove this rule

return (
<button className={classnames(type, className)} {...props}>
<Svg name={type} />
K?
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking that the button could be a simple ? (since im used to it from other editors) with no K (since K is not necessarily meaningful).. not sure, what do you think?

@jasonLaster
Copy link
Contributor

Hey @Subtlesnow do you think you'll have time to make these changes?

@Subtlesnow
Copy link
Author

Subtlesnow commented Oct 5, 2017 via email

@jasonLaster
Copy link
Contributor

Thanks @Subtlesnow!

@jasonLaster
Copy link
Contributor

Hey @Subtlesnow how's your availability?

@Subtlesnow
Copy link
Author

Subtlesnow commented Oct 11, 2017 via email

Copy link
Contributor

@wldcordeiro wldcordeiro left a comment

Choose a reason for hiding this comment

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

Remove the package-lock.json file. We're using yarn's lockfile. On that note should we .gitignore the package-lock.json file to avoid this in the future @jasonLaster

@jasonLaster
Copy link
Contributor

Hey @Subtlesnow, I re-opened your PR on #4382 w/ some small tweaks. Thanks for the help at the meetup!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants