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

Feature/google search box #177

Closed
wants to merge 22 commits into from
Closed

Conversation

imsauravsingh
Copy link
Collaborator

Working on all test cases.

import type { Props } from './types';

const Search = ({ className, inputProps, dataLists, renderListItem }: Props): Node => {
const [inputValue, setInputValue] = useState(inputProps.value || '');
Copy link
Collaborator

Choose a reason for hiding this comment

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

inputValue --> searchString looks more contextual

searchText => {
const searchValue = typeof searchText === 'string' ? searchText : inputValue;

if (searchValue) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this logic I believe would come from TypeAhead

[inputValue, dataLists, setSearchList]
);

const onResult = result => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make onResult more explicit? its ambiguous what it does

onSpeachRecognitionResults makes it more clear

handleSpeechEnd,
});

const onSpeechClick = () => onListen();
Copy link
Collaborator

Choose a reason for hiding this comment

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

onListen . --> onSpeechListenHandler

<div className="search-button">
<Button>
{isListening ? (
<i className="search-icon search-loader" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use Icon component

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes

<div className="search-box">
<Input
{...inputProps}
aria-label="Search"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Labels should never be hard coded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, I'll update

overflow: hidden;
}

button {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is wrong don't style the component here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Once Icon componet will be exposed then unnecessary codes will be removed.

}

.search-mic {
background: url(images/googlemic.png) 0 0/24px no-repeat;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not a good approach

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a part of the icon component. Once Icon component will be exposed then these codes will be removed.

@vinodloha vinodloha added the invalid This doesn't seem right label Dec 11, 2019
@vinodloha vinodloha removed the invalid This doesn't seem right label Dec 13, 2019
@vinodloha vinodloha closed this Jan 8, 2020
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.

None yet

2 participants