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

Left Sidebar Improvements #19

Merged
merged 8 commits into from
Jan 17, 2022
Merged

Conversation

ghislaineguerin
Copy link
Contributor

@ghislaineguerin ghislaineguerin commented Jan 6, 2022

This spec goes over the design and implementation notes for Left Pane Improvements

@ghislaineguerin ghislaineguerin changed the title Left Sidebar Component Improvements Left Sidebar Improvements Jan 7, 2022
@ghislaineguerin ghislaineguerin marked this pull request as ready for review January 7, 2022 12:49
@ghislaineguerin ghislaineguerin requested a review from a team as a code owner January 7, 2022 12:49
@github-actions github-actions bot requested review from eito-fis, kgodey, mathemancer, pavish, seancolsen and silentninja and removed request for a team January 7, 2022 12:50
Copy link
Contributor

@seancolsen seancolsen left a comment

Choose a reason for hiding this comment

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

This looks great. It seems like it's very similar to the VS Code sidebar which I like -- not because our users will necessarily be familiar with VS Code -- but because I think part of the reason VS Code has gained popularity so quickly is that its UX is very approachable.

Questions

  • In the explorer, when I enter a search query like "foo" with the filter below set to "Tables", will it also show views that match "foo"? Or will the search respect the filter?

Suggested formatting changes

for this spec and for future ones too

  • I'd prefer to see just one heading per scenario. Here we have three headings per scenario which seems superfluous.
  • I'd prefer the scenarios to be a flat list. Instead of 1a, 1b, I'd prefer to have 1, 2, and so on.

Copy link
Member

@pavish pavish left a comment

Choose a reason for hiding this comment

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

Looks good to me

@pavish pavish removed their assignment Jan 7, 2022
@seancolsen seancolsen removed their assignment Jan 7, 2022
@kgodey
Copy link
Contributor

kgodey commented Jan 7, 2022

@ghislaineguerin This looks good to me.

Question: The original issue, mathesar-foundation/mathesar#723 says:

All tables and views from the db are listed on the left pane. This would become impractical to use if the number of tables/views is large (>50). We require an improved UX where we do not show all the tables/views.

I don't see any reference to handling large numbers of tables or views in the spec. What is the provision for this?

@kgodey kgodey removed their assignment Jan 7, 2022
@ghislaineguerin
Copy link
Contributor Author

@kgodey I didn't include any special UI for partial loading of content in the case of long lists and we decided not to address that now (when we worked on the design for listing views). We will have basic scrolling and if there are both tables and views, then the user should see both sections, meaning that a section doesn't get pushed down. That's implemented in the prototype.

@silentninja
Copy link
Contributor

I like the sidebar design, glad to see that it won't be used just for navigation. Will it be possible to see the information of the view such as the table and columns it is derived from in a split panel below the view listings? By split panel, I mean the place where tables are listed when viewing the All section

@kgodey
Copy link
Contributor

kgodey commented Jan 13, 2022

@kgodey I didn't include any special UI for partial loading of content in the case of long lists and we decided not to address that now (when we worked on the design for listing views).

@ghislaineguerin I thought we weren't handling it in listing views because we were going to handle it here, it's part of the issue description for mathesar-foundation/mathesar#723. If you're not handling all the requirements from mathesar-foundation/mathesar#723, please open a separate issue for whatever you're not handling and link it from the original issue. I would also put a note in the spec so that people who are comparing the spec and the issue are not confused about why it doesn't match up with all the requirements.

@silentninja silentninja removed their assignment Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants