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

Feat: Add displayTrigger boolean #1332

Merged
merged 4 commits into from
Aug 27, 2022
Merged

Feat: Add displayTrigger boolean #1332

merged 4 commits into from
Aug 27, 2022

Conversation

mcrowder65
Copy link
Contributor

@mcrowder65 mcrowder65 commented Aug 12, 2022

Add null option to props.trigger, so that the trigger won't render. ex:

<div id="main">
      <CommandPalette commands={commands} reactModalParentSelector="#main" trigger={null} open />
</div>   

@@ -443,17 +450,17 @@ CommandPalette.propTypes = {
* first suggestion. Defaults to false. */
highlightFirstSuggestion: PropTypes.bool,

/** When suggestion is clicked, react-autosuggest needs to populate the input element
/** When suggestion is clicked, react-autosuggest needs to populate the input element
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why these are appearing. They're not in my commit locally.

@asabaylus asabaylus added enhancement minor Increment the minor version when merged labels Aug 14, 2022
@asabaylus asabaylus self-assigned this Aug 14, 2022
@asabaylus
Copy link
Owner

asabaylus commented Aug 14, 2022

@mcrowder65 the snapshot tests are failing and we need some docs in the readme after this can be merged. Out of curiosity why not use something like the following?

<ReactCommandPalette trigger={<div />} commands={commands} />

my suggestion for the README.md

displayTrigger a boolean that prevents the trigger from rendering. Useful when the command pallet will be opened externally.

@asabaylus asabaylus changed the title Add displayTrigger boolean Feat: Add displayTrigger boolean Aug 14, 2022
@mcrowder65
Copy link
Contributor Author

@asabaylus why not allow null for the trigger? I don't see why it needs to have a div at all?

@asabaylus
Copy link
Owner

@asabaylus why not allow null for the trigger? I don't see why it needs to have a div at all?

Good point, did you want to update the PR or shall I?

@mcrowder65
Copy link
Contributor Author

@asabaylus addressed!

@asabaylus
Copy link
Owner

asabaylus commented Aug 21, 2022

@mcrowder65, there are several failing tests. The pallet needs to render unless null is set, ex:

if(trigger !== null) {
     return (
       <div className="react-command-palette">
         <PaletteTrigger
           onClick={this.handleOpenModal}
           trigger={trigger}
           theme={theme.trigger}
         />
         {modal}
       </div>
     );
   }
   
   return modal;

We still need a unit test.

@codecov
Copy link

codecov bot commented Aug 21, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (a4d3b9d) compared to base (85f1e14).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1332   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          218       221    +3     
  Branches        35        36    +1     
=========================================
+ Hits           218       221    +3     
Impacted Files Coverage Δ
src/command-palette.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@asabaylus asabaylus merged commit f9392bc into asabaylus:main Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants