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

Further Query Improvements #2245

Merged
merged 12 commits into from
Feb 1, 2018
Merged

Conversation

kocsmy
Copy link
Collaborator

@kocsmy kocsmy commented Jan 19, 2018

Initial commit for #2234

@kocsmy kocsmy changed the title Attach refresh tot he bottom Further Query Editor Improvements for issue #2234 Jan 19, 2018
@kocsmy kocsmy changed the title Further Query Editor Improvements for issue #2234 Further Query Editor Improvements Jan 19, 2018
@kocsmy kocsmy changed the title Further Query Editor Improvements Further Query Improvements Jan 19, 2018
@kocsmy
Copy link
Collaborator Author

kocsmy commented Jan 24, 2018

Thanks for the cleanup and layout improvements, @kravets-levko

I can spot 2 issues:

  • Minimalising the query editor makes the top part of the visualisation go below the hidden area and disappear:
    screenshot 2018-01-25 00 15 30

  • When the editor is closed, the metadata and meta refresh and history goes up. It should still stick to the bottom:
    screenshot 2018-01-25 00 15 41

@kravets-levko
Copy link
Collaborator

@kocsmy Fixed. Can you please check and confirm that everything is ok

@kocsmy
Copy link
Collaborator Author

kocsmy commented Jan 25, 2018

Thanks for the quick fixes, @kravets-levko — all look good now!

@arikfr
Copy link
Member

arikfr commented Jan 28, 2018

Some comments:

Important:
The data area needs its own scroll - right now the scroll is shared between the data area and the query text. This has two down sides: when you scroll the data you can't see the query but more important: you need to scroll down to be able to scroll to the side.

Nice to have: (can be fixed later on, unless very simple)

  • The refresh schedule is part of the query metadata, so it doesn't need a divider on top of it. The vertical divider on the right should be seen on it.
    image

  • In data only view, the left side is very empty. Maybe we should use a horizontal area for metadata in such case?

@kocsmy
Copy link
Collaborator Author

kocsmy commented Jan 28, 2018

Thanks @arikfr

  • Regarding the scroll: yes, we discussed that with @kravets-levko and we weren't sure, I think he can implement that version.

  • Refresh schedule: You mean like this?
    screenshot 2018-01-28 13 20 02

  • Too empty left side: I wouldn't move the bottom fixed metadata to the top, I actually tried that and having it move around is more annoying than having some empty space there. Maybe we can add something there later.

@arikfr
Copy link
Member

arikfr commented Jan 28, 2018

Refresh schedule: You mean like this?

Yes and probably with less padding between it and the "created/updated" part.

Too empty left side: I wouldn't move the bottom fixed metadata to the top, I actually tried that and having it move around is more annoying than having some empty space there. Maybe we can add something there later.

That's because you think about this as being the same page in two different states. But what if you think about it as an independent page?

@kocsmy
Copy link
Collaborator Author

kocsmy commented Jan 28, 2018

That's because you think about this as being the same page in two different states. But what if you think about it as an independent page?

But this is the same page in different state?:) Okay, if I make myself think about that way, then we don't need the left sidebar at all and we can go with a totally different layout. But afaik we just moved away from that and made a decision to show the schema section always, even if it's empty? Anyway, I got what you're saying and I agree that left side feels very empty. Not sure how to address this, but let's discuss on our call.

@arikfr
Copy link
Member

arikfr commented Jan 31, 2018

This PR needs a rebase...

@kocsmy
Copy link
Collaborator Author

kocsmy commented Jan 31, 2018

@arikfr this seems to be done and conflicts are resolved. It's ready for review and merge.

@arikfr arikfr merged commit 34d22ee into getredash:master Feb 1, 2018
@arikfr
Copy link
Member

arikfr commented Feb 1, 2018

There is still one thing left and it's the issue with the "Refresh Schedule" not being part of the metadata above it. I merged regardless so we don't keep this large pull request hanging.

Created #2279 to track this.

@kocsmy
Copy link
Collaborator Author

kocsmy commented Feb 1, 2018

Thanks @arikfr — yes, that's totally kept in mind and we'll make that happen, too!

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.

3 participants