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

Make table visualization header fixed #5103

Merged
merged 7 commits into from Aug 17, 2020
Merged

Make table visualization header fixed #5103

merged 7 commits into from Aug 17, 2020

Conversation

gaecoli
Copy link
Member

@gaecoli gaecoli commented Aug 13, 2020

What type of PR is this? (check all applicable)

  • Feature

Description

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

@gaecoli
Copy link
Member Author

gaecoli commented Aug 13, 2020

If you want to add this feature to query page, you just add this css to query page.

@gabrieldutra gabrieldutra self-requested a review August 14, 2020 15:21
Copy link
Member

@gabrieldutra gabrieldutra 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 your contribution @yankeeguyu 😁, I synced your branch with master so we can check it in our preview environment and moved the styling into a new className. I may make one more adjustment into the border before merging this.

@gaecoli
Copy link
Member Author

gaecoli commented Aug 15, 2020

Ok, thanks, but i found the table header is transparent, do you have some idea to change it?
Like this:
image

I know this is a hover style, but i change it, it didn't work. If you have free time, please reply me, thanks a lot!

@gaecoli
Copy link
Member Author

gaecoli commented Aug 15, 2020

And another question, this feature can be add in query page? I can't find query style. Thank you!

@gaecoli
Copy link
Member Author

gaecoli commented Aug 15, 2020

Ok, thanks, but i found the table header is transparent, do you have some idea to change it?
Like this:
image

I know this is a hover style, but i change it, it didn't work. If you have free time, please reply me, thanks a lot!

This solved by backgroud: #fafafa !important, it will override element.style.

Copy link
Member Author

@gaecoli gaecoli left a comment

Choose a reason for hiding this comment

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

Please add this.

left: 0;
top: 0;
border-top: hidden;
background: #fafafa !important;
Copy link
Member Author

Choose a reason for hiding this comment

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

review this code, if didn't add !important, the table header will show this:
image
It will be hover override.

Copy link
Member

Choose a reason for hiding this comment

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

NP, we can keep the !important, I was actually gonna ask you the reason for both that and the z-index.

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha...your NP means Chinese good? Thanks! At first i added z-index because of my wrong idea, now i want to correct this. I didn't test all feature, it's my fault.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, NP = No Problems.

Don't worry, I already removed the z-index. Considering this is something we wanted for quite a long time, I'll just try to refine the border myself tomorrow (it has a 1 px border that doesn't follow the sticky) and after some more tests it should be ready to go 🙂.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, NP = No Problems.
Don't worry, I already removed the z-index. Considering this is something we wanted for quite a long time, I'll just try to refine the border myself tomorrow (it has a 1 px border that doesn't follow the sticky) and after some more tests it should be ready to go 🙂.

Ok, if you perfect this feature, tell me, i want to add in my online redash. Thank you.

@gabrieldutra
Copy link
Member

gabrieldutra commented Aug 15, 2020

And another question, this feature can be add in query page? I can't find query style. Thank you!

It should be working in the query page too, as that styling is applied to both pages

@gabrieldutra
Copy link
Member

But for the query page it seems to only work for the desktop version, which has a fixed area for the visualization as well as the Dashboard version

@gabrieldutra
Copy link
Member

gabrieldutra commented Aug 16, 2020

I install Redash's version is 8.0.0 by npm and python celery, this version has another way to add this style to query page?

We had a big restyling in the Query Pages after v9-beta, so it's probably the reason it behaves differently here.

@gaecoli
Copy link
Member Author

gaecoli commented Aug 16, 2020

I install Redash's version is 8.0.0 by npm and python celery, this version has another way to add this style to query page?

We had a big restyling in the Query Pages after v9-beta, so it's probably the reason it behaves differently here.

Ok, i knew it.

Copy link
Member Author

@gaecoli gaecoli left a comment

Choose a reason for hiding this comment

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

viewed.

Thanks, this change is good to me.

Co-authored-by: Gabriel Dutra <nesk.frz@gmail.com>
@gaecoli
Copy link
Member Author

gaecoli commented Aug 17, 2020

Thanks @gabrieldutra , it's a very pleasant PR.

@arikfr
Copy link
Member

arikfr commented Aug 17, 2020

@gabrieldutra will this be compatible with Ant v4?

Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

From a product perspective this looks good. Thank you, @yankeeguyu !

@gabrieldutra @kravets-levko up to you to decide when to merge based on code review.

@gaecoli
Copy link
Member Author

gaecoli commented Aug 17, 2020

From a product perspective this looks good. Thank you, @yankeeguyu !

@gabrieldutra @kravets-levko up to you to decide when to merge based on code review.

Thank you. My honor.

@gaecoli
Copy link
Member Author

gaecoli commented Aug 17, 2020

From a product perspective this looks good. Thank you, @yankeeguyu !

@gabrieldutra @kravets-levko up to you to decide when to merge based on code review.

At last, if this feature adds into query page, it will be perfect!

@gabrieldutra
Copy link
Member

@arikfr for me this is good to go, I've already made all the updates I wanted 🙂. As for Antd v4, I've just tested, it works, I might need to tweak class names, but will sync after merge.

Another thing to be handled is the Search feature:
search-fixed-header

But I can do this one in a follow up PR, or in the Antd v4 one (I think it's easier as I moved the Search to above the header)

@gabrieldutra gabrieldutra changed the title add lock table header in dashboard page Make table visualization header fixed Aug 17, 2020
@gabrieldutra gabrieldutra merged commit 83c6a6b into getredash:master Aug 17, 2020
@gabrieldutra
Copy link
Member

Merged, thank you very much @yankeeguyu 🎉.

Also, consider upgrading your Redash version, it seems you already have a couple of reasons to do that :)

@gaecoli
Copy link
Member Author

gaecoli commented Aug 17, 2020

Merged, thank you very much @yankeeguyu 🎉.

Also, consider upgrading your Redash version, it seems you already have a couple of reasons to do that :)

Ok, i will consider it.

@gaecoli
Copy link
Member Author

gaecoli commented Sep 3, 2020

@arikfr for me this is good to go, I've already made all the updates I wanted 🙂. As for Antd v4, I've just tested, it works, I might need to tweak class names, but will sync after merge.

Another thing to be handled is the Search feature:
search-fixed-header

But I can do this one in a follow up PR, or in the Antd v4 one (I think it's easier as I moved the Search to above the header)

Hi @gabrieldutra , you moved Seach to above header? If you did, i want to use your PR, thanks!

@gabrieldutra
Copy link
Member

@yankeeguyu you can check f53ac0d, but adding a z-index to the table header should fix your problem too.

@gaecoli
Copy link
Member Author

gaecoli commented Sep 4, 2020

@yankeeguyu you can check f53ac0d, but adding a z-index to the table header should fix your problem too.

Thank you @gabrieldutra . I solved it.

andrewdever pushed a commit to andrewdever/redash that referenced this pull request Oct 5, 2020
* add lock table header

* Move styling to a new class

* Update renderer.less

* Move class to table and fix top border

* Update renderer.less

* Update viz-lib/src/visualizations/table/renderer.less

Thanks, this change is good to me.

Co-authored-by: Gabriel Dutra <nesk.frz@gmail.com>

Co-authored-by: Gabriel Dutra <nesk.frz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants