-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add reactive search for API catalog #3502
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general comments:
- making a new branch
- use the current way of naming new branch. Here it would have been something like: feature/add-reactive-search-for-apis
- Title of PR is generated from branch name, so it may need some adjust to be human language. Here something like: Add reactive search for API catalog
- PR template
- closes keyword
- add number of issue to be closed. Here Add reactive search for API catalog #3485
- changes: list changes done in general level
- closes keyword
// Init the query reactive variable | ||
instance.query = new ReactiveVar(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra empty line.
<template name="search_form"> | ||
<form id="search-form"> | ||
<div class="input-group"> | ||
<span class="input-group-addon"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the span from here.
The goal is to have same feel and look as in Dashboard page search component.
@@ -463,6 +463,7 @@ | |||
"mostFrequentUsersTable_lintText_downloadFile": "Lataa kaikki k\u00e4ytt\u00e4j\u00e4t", | |||
"navbar_addAPIBackend": "Lis\u00e4\u00e4 rajapinta", | |||
"navbar_apis": "Rajapinnat", | |||
"navbar_my_apis": "Omat sovellusliittymät", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not make any changes in fi.i18n.json.
The changes will be done afterwards during localization round.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matleppa so should simply use english word for then text
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinaytech When you are adding a new label, text block etc. or modifying existing one, just make those changes only in en.i18n.json, in English language.
Just pay no attention in file fi.i18n.json. (So in this case, remove the changes you've done there now).
The corresponding changes will be translated later and generated with another tool to Finnish translation file.
</div> | ||
</form> | ||
<div class="container"> | ||
<form id="search-form"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated in Contributing Guidelines:
"Indent all HTML(Blaze) and JavaScript code with two spaces for each level of nesting."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting closer, just some indentation fixes left.
<div class="col-xs-12 col-sm-4"> | ||
<div class="search"> | ||
<i class="fa fa-search"></i> | ||
<input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary indentation level for input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i will format it
General comments:
|
Closes #3485
Changes
Developer checklist
This checklist is to be completed by the PR developer:
Reviewer checklist
Reviewed by: @username1
This list is to be completed by the pull request reviewer: