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

Typography fixes for merged Branding PR's #196

Merged
merged 6 commits into from
Jul 5, 2019
Merged

Conversation

ricardobaeta
Copy link
Contributor

This PR fixes the identified issues on the UI, after we merged Branding typography PR #139 and Adds UI Elements Branding to UI PR #142

The issues solved are listed bellow:

Signed-off-by: Ricardo Baeta [email protected]

This commit fixes the issues listed bellow, after we merged the Typography PR.

1) #185
2) #184
3) #183
4) #186

I'll be doing another commit to solve #192 and then will create a PR.

Signed-off-by: Ricardo Baeta <[email protected]>
This fixes

UAST tab changes some global styles #183

Signed-off-by: Ricardo Baeta <[email protected]>
Copy link
Contributor

@dpordomingo dpordomingo left a comment

Choose a reason for hiding this comment

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

I see no technical problems but I hadn't time to test it locally

@dpordomingo dpordomingo requested a review from a team July 4, 2019 12:18
@dpordomingo dpordomingo added the bug Something isn't working label Jul 4, 2019
@carlosms
Copy link
Contributor

carlosms commented Jul 4, 2019

The other issues listed seem to be fixed.

But the sql lab results is not working properly right now.

In this PR, when the table is too tall, the scrollbar is half hidden. If I try to scroll to the bottom of the table, I can't. Without the outer scrollbar some contents just disappear.
127 0 0 1_8088_superset_sqllab (3)

When the results table is too wide I can't see the bottom scrollbar:
127 0 0 1_8088_superset_sqllab

This is how it looks in v0.3.0-rc.1.

Too tall:
127 0 0 1_8088_superset_sqllab (2)

Too wide:
127 0 0 1_8088_superset_sqllab (1)

And in master (97fcccf). Here you can see the bottom is just a little bit clipped, the horizontal scrollbar is half-hidden.

Too tall:
tall

Too wide:
wide

@carlosms
Copy link
Contributor

carlosms commented Jul 4, 2019

In case it wasn't clear in my previous comment, I think #184 is something that we can improve, but at worst is not much than an inconvenience. While the changes in this PR can be considered a bug.
I'll block this PR as it is, but I would be OK removing the changes for the results table and fixing #184 separately at some point later.

@ricardobaeta
Copy link
Contributor Author

Hi @carlosms Thanks for the prompt review! No need to block this PR. The issues with #184 are fixed by my latest commit 9efe159. Look at 👇

query-table-vertical-scroll

query-table-horizontal-scroll

@carlosms
Copy link
Contributor

carlosms commented Jul 4, 2019

After testing the last commit 9efe159, I've noticed 2 problems.

  1. There is always a horizontal scrollbar, even when the results fit.

9efe159:
fits

v0.3.0-rc.1
fits-prev

When the results table is too tall, the vertical scroll gets hidden:
tall

  1. The results table now has a fixed height that does not make use of the whole window.

In 9efe159:
tall-prev

In v0.3.0-rc.1: See the gif used in point 1, it can be seen that the table has a fixed height and a lot of white space below it.

.

Since this doesn't look like a straight forward fix, I'd suggest to remove any related code from this PR, leave #184 as a known issue for now since it's mostly a cosmetic thing, and fix it independently later.

Signed-off-by: Ricardo Baeta <[email protected]>
@ricardobaeta
Copy link
Contributor Author

ricardobaeta commented Jul 4, 2019

Thank you once again @carlosms. My latest commit 30fb5e7 removed the #184 related code, and I agree to tackle this issue on a further PR.

@ricardobaeta
Copy link
Contributor Author

ricardobaeta commented Jul 5, 2019

Thanks @carlos! I already pushed a commit 40f95cc with min-width.

@ricardobaeta
Copy link
Contributor Author

ricardobaeta commented Jul 5, 2019

Thanks @carlos! I already pushed a commit 9884af9 with min-width.

@carlosms carlosms requested a review from dpordomingo July 5, 2019 10:32
Copy link
Contributor

@dpordomingo dpordomingo left a comment

Choose a reason for hiding this comment

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

LGTM,
I only found this issue #199, but was not created by this PR, so I'd say merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants