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

issue-113: Add share search #114

Closed
wants to merge 4 commits into from
Closed

Conversation

nikhilj13
Copy link
Contributor

Adds a share search icon next to the SearchBar. This icon is hidden by default but if you set the sharesearch prop for SearchBar to true, you'll be able to see it.

Here's an example:
ShareSearch

@nikhilj13 nikhilj13 changed the title issue-113: Added share search issue-113: Add share search Apr 24, 2019
@klk1010 klk1010 added the merge conflicts This PR has a merge conflict that needs to be resolved. label Jul 22, 2019
@nikhilj13 nikhilj13 removed the merge conflicts This PR has a merge conflict that needs to be resolved. label Jul 23, 2019
Copy link
Collaborator

@luigivaldez luigivaldez left a comment

Choose a reason for hiding this comment

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

I think it makes more sense to have this be a completely separate component from the SearchBar. On the search page (in Search UI), you can then include it right after the <SearchBar> component. but this way you don't have to complicate SearchBar and you don't have to pass all of the properties you want this component to have through the SearchBar's properties.
Does that make sense?

@@ -283,6 +288,18 @@ class SearchBar extends React.Component<SearchBarDefaultProps, SearchBarProps, S
}
}

//eslint-disable-next-line
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the eslint-disable-next-line here?

shareSearch() {
const username = AuthUtils.getUserName(AuthUtils.getSavedUser());
const yourMessage =
'Hey,\n\nI think you would be interested in these search results that I found using the most cognitive and ' +
Copy link
Collaborator

Choose a reason for hiding this comment

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

This message should be a property. In a PoV this makes sense, but if someone wants to use it in a production system, they'd probably want to have different text.

`intuitive search platform called Attivio. Here is the link :)\n${window.location.href}
\nLet me know if you have any questions!\n\nBest,\n${username}`;
const subject = 'Search results I found using Attivio!';
const emailLink = '[email protected]';
Copy link
Collaborator

Choose a reason for hiding this comment

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

The target email should be a property too, although it should default to being empty, I think... In general, it doesn't make sense to have the address come up pre-filled to something no one will use... (for a PoV, sure...)

const shareSearch = this.props.shareSearch ? (
<span
className="attivio-smalltoolbar-btn"
title="Share this search via Email"
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Email" shouldn't be capitalized.

@klk1010 klk1010 added the help wanted Extra attention is needed label Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants