-
Notifications
You must be signed in to change notification settings - Fork 89
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
Space out header #379
Space out header #379
Conversation
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.
Looks good so far. When you finish the CSS, I will do a thorough review and approve.
@domoritz |
@domoritz I am quite fine with adding the Share URL button one level deep inside export. But I am open to changes. |
We had a lengthy discussion about this on Slack. It would be better to have it at the top level since |
@domoritz |
b125978
to
50a364c
Compare
@domoritz Shall we merge this? |
|
@siddhant1 We also need the help dialog cleanup for these changes (e.g. we want to docs links to be at the top of the help dialog). |
@domoritz Format works if you run it through the command palette. |
@domoritz It initially too worked like this. I think it is a problem with the Monaco editor's formatter. |
@domoritz I am thinking to modify it like this: |
|
@domoritz Check the help modal too |
Looks good. Let's move the docs links up. |
@domoritz Done! |
Format still doesn't work correctly for me. I unchecked #379 (comment). Can you move the references above the shortcuts? |
62237ab
to
23376de
Compare
This looks too good! |
Co-Authored-By: Dominik Moritz <domoritz@gmail.com>
77fe370
to
5db537c
Compare
fde253c
to
6744117
Compare
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.
Bugs I found:
- The run button doesn't catch clicks on the whole button.
- Cannot run at all in automatic mode.
- Switching from manual to automatic moves the
v
I am not sure if I get this. What is the problem exactly? |
43eef25
to
93cd5e5
Compare
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.
Just two small things and then we are good to go.
Header
onSelectNewVegaLite
is being repeated in filesrc/components/input-panel/spec-editor/renderer.tsx
andsrc/components/header/renderer.tsx
.WIP
since the header requires some minor CSS fixes to improve spacing between the buttons.