Skip to content

Conversation

chrisrichie25
Copy link
Contributor

@chrisrichie25 chrisrichie25 commented Aug 2, 2021

Description

In this PR, two props (showQueryHistoryButton and showExportToLanguageButton) were added to enable hiding the two respective buttons from the compass-query-bar as it is being used. Also, documentation was updated to make it more clear how to run the compass application after cloning the repository.

Checklist

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

Motivation and Context

The data explorer feature in the MongoDB DataExplorerFindView is starting to use the compass-query-bar plugin instead of the previous regular html input element for users to input their queries so as to support more options for queries. Currently, we are not supporting the query history and export to language features that are present in the compass-query-bar. As a result, we wanted to disable and hide the two buttons corresponding to these features from the compass-query-bar as we use it. This PR does exactly that.

  • 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)

Chrispine Lwekaza added 2 commits August 2, 2021 09:58
…ton and showExportToLanguageButton props so that the user of the component can choose to show or hide these buttons. Update the tests to make sure these props work
Copy link
Collaborator

@gribnoysup gribnoysup 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! Only a few small notes on the README update

README.md Outdated

- `npm run compile-changed` will compile all plugins and their dependants changed since `origin/HEAD`
- `npm run test-changed` will run tests in all packages and their dependants changed since `origin/HEAD`
- `npm run test-changed` will run tests in all packages and their dependants changed since `origin/HEAD`. To run all tests in one plugin that you are working on such as the `compass-connect` plugin, you can run `npm run test` from the top of the `compass/packages/compass-connect` directory.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about we move this additional note to the section above where all the instructions for different way of running a script already present?

@gribnoysup gribnoysup changed the title CLOUDP-96456 feat(@mongodb-js/compass-query-bar): Add props to toggle visibility of "query history" and "export to language" buttons CLOUDP-96456 Aug 3, 2021
chrisrichie25 and others added 2 commits August 3, 2021 09:20
Update README.md to have more concise directions for running compass locally

Co-authored-by: Sergey <sergey.petushkov@protonmail.com>
Rework a paragraph to create a better organization of the documentation as the comment suggested.
Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

lgtm! Nice readme updates 👍

Screen Shot 2021-08-03 at 11 49 48 AM

store={store}
actions={actions}
layout={layout}
showQueryHistoryButton
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is false by default do we also need to enable it where is used in Compass?

Copy link
Collaborator

@gribnoysup gribnoysup Aug 4, 2021

Choose a reason for hiding this comment

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

I think it's true by default (see previous test and defaultProps in the component), so this test is just a bit redundant (also a bit confusing because of that 😅 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcasimir Sorry I missed your comment earlier. showQueryHistoryButton and showExportToLanguageButton props are true by default as @gribnoysup commented. I did that so that I would not change the appearance of the compass query bar out of the box, but that we would also have the option to disable it when we want to. The redundant test you are referring to is present just make sure that those buttons are present when these two props are not explicitly set. I am happy to remove them if you think that would be better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think even thought it's a bit redundant, it's okay to keep it there 😊

@gribnoysup gribnoysup merged commit 76acacc into mongodb-js:main Aug 4, 2021
@chrisrichie25 chrisrichie25 deleted the CLOUDP-96456 branch August 4, 2021 13:03
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