Skip to content

Conversation

mabaasit
Copy link
Collaborator

@mabaasit mabaasit commented Mar 11, 2022

this PR fixes the broken test that was introduced in #2867

Due to less scroll area of sidebar databases, the database.collection is not visible in the viewport and test fails.
Now changed search param to collection name.

Description

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@mabaasit mabaasit requested a review from lerouxb March 11, 2022 13:47
@lerouxb
Copy link
Contributor

lerouxb commented Mar 11, 2022

lol

it('can create a collection and drop it', async function () {
const dbName = 'test'; // existing db
const collectionName = 'my-sidebar-collection';
const collectionName = 'a-sidebar-collection';
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just search in the sidebar so it is the only one being displayed?

Copy link
Contributor

Choose a reason for hiding this comment

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

see helpers/commands/navigate-to-collection-tab.js for how to do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah damn. i didn't know that we can also search by collection name. that sounds much better. I always tried db.collection but of course it didn't work.

@mabaasit mabaasit requested a review from lerouxb March 11, 2022 14:24
Copy link
Contributor

@lerouxb lerouxb left a comment

Choose a reason for hiding this comment

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

I'm happy with this either way, btw. A third way to fix this would be to extend helpers/compass/scroll-to-virtual-item.ts to work with the sidebar which I think also uses virtual scrolling.

There might be a race condition with searching: if you search before the collection exists it will never find it. In that case your original solution of putting it at the front is fine for now.

@lerouxb
Copy link
Contributor

lerouxb commented Mar 11, 2022

I don't mind trying to fix this as well - I probably broke it when I added lots of collections. I think the way to go is to make helpers/compass/scroll-to-virtual-item.ts work with the sidebar.

@mabaasit mabaasit changed the title fix: change collection name to appear first fix: search by collection name Mar 14, 2022
@mabaasit mabaasit merged commit 388f073 into main Mar 14, 2022
@mabaasit mabaasit deleted the fix-sidebar-test branch March 14, 2022 09:26
@mabaasit mabaasit mentioned this pull request Mar 14, 2022
10 tasks
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