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

Allow for disabling/overriding of search modal from a plugin #1561

Merged
merged 17 commits into from
Nov 15, 2016

Conversation

brent-hoover
Copy link
Collaborator

@brent-hoover brent-hoover commented Nov 10, 2016

Resolves #1399

Provide a registry setting providing "ui-search" and a template name and that will be used. Don't provide it or don't enable one? No search button.

@@ -6,8 +6,9 @@
<div class="menu">
{{> tagNav tagNavProps}}
</div>

<div class="search">{{> React IconButtonComponent}}</div>
{{#if isSearchEnabled}}
Copy link
Contributor

@aaronjudd aaronjudd Nov 11, 2016

Choose a reason for hiding this comment

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

Since we are still using Blaze here, why not use

  {{#each reactionApps provides='ui-search' enabled=true}}
  {{#each}}

(or some variation using the reactionApps helper..)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because I already needed to query on the db to get the template and I thought it made the code there easier to read.

Copy link
Contributor

@aaronjudd aaronjudd Nov 11, 2016

Choose a reason for hiding this comment

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

Reaction Apps returns the template, as well as i18n values and checks permissions etc.

Reaction.Apps({provides: "ui-search"}).template;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to use Reaction.Apps

@aaronjudd
Copy link
Contributor

👍 conditionally approved. The issue here is that all the components could use Reaction.Apps, not just the icon, and a shortcut registry entry for the icon. The implementation as it stand now, is just a boolean visibility, versus the ability to have multiple potential templates and display only active ones. We'll need to review this more, but @mikemurray will hit this in update the nav bar to reaction components and the template registry.

@aaronjudd aaronjudd merged commit 9ace63d into release-0.18.0 Nov 15, 2016
@aaronjudd aaronjudd deleted the brent-fix-issue-1399 branch November 15, 2016 19:25
@aaronjudd aaronjudd removed the review label Nov 15, 2016
@spencern spencern mentioned this pull request Oct 11, 2017
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