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

Add collapse to sidebar #380

Merged
merged 2 commits into from
Nov 11, 2017
Merged

Add collapse to sidebar #380

merged 2 commits into from
Nov 11, 2017

Conversation

haivp3010
Copy link
Contributor

This pull request targets issue #317.
A collapse bar is put just to the left of the sidebar, making use of the space between the sidebar and the window border.

sidebar

Copy link
Member

@maxcnunes maxcnunes left a comment

Choose a reason for hiding this comment

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

The code looks good to me. I will test it properly later this week though.

@maxcnunes
Copy link
Member

Sorry @haivp3010, I have been traveling and I have not been having free time to review it. I will try to find someone else who can do it.
Could someone (@BornaP @s-kem) review it, please?

@BornaP
Copy link
Member

BornaP commented Nov 6, 2017

I'll take a look

@BornaP
Copy link
Member

BornaP commented Nov 6, 2017

Hey @haivp3010 thanks for Pull requests. There's an issue though. When you execute some query and collapse menu afterwards, part of the table is whitened out:

screen shot 2017-11-06 at 23 18 20

Could you please take a look :)

@haivp3010
Copy link
Contributor Author

I think the problem is with window resize itself, so maybe it needs a separate issue/PR. I will look into that and confirm later today.

@s-kem
Copy link
Contributor

s-kem commented Nov 7, 2017

@haivp3010 I'm seeing a couple things here...

query-result-table ends up inheriting props.widthOffset from query-browser's state.sideBarWidth. Even when the sidebar is collapsed, the state is still the actual width of the sidebar, so it's still offsetting by that value.

A ternary around line 514 on the widthOffset prop sent to Query fixes it -- widthOffset={this.state.sidebarCollapsed ? 0 : this.state.sideBarWidth}

Simply setting sideBarWidth to 0 on onCollapseClick seems to disable your collapsing, I assume due to your use of negative margin.

However this only works when you collapse the sidebar first and then execute the query. It appears there's also an overall re-rendering issue in query-result-table not re-rendering when props.widthOffset changes. I'm noticing the table not resizing when I drag resize the sidebar.

@s-kem
Copy link
Contributor

s-kem commented Nov 7, 2017

I made a PR, #395 that solves the re-render issue. As long as you're passing a 0 offset down when collapsing the sidebar it should cause either a re-render or a recompute of the grid size to happen.

@BornaP
Copy link
Member

BornaP commented Nov 9, 2017

Cool. I'll check it today or tomorrow afternoon (European time ;) )

@BornaP
Copy link
Member

BornaP commented Nov 11, 2017

Since @s-kem has resolved re-rendering issue, I'll merge this PR. Thank you @haivp3010 👏

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.

4 participants