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.

Sign up for GitHub

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

Jump to bottom

Viewed By Customer #20

Merged
merged 7 commits into from
Aug 7, 2019
Merged

Viewed By Customer #20

merged 7 commits into from
Aug 7, 2019

Conversation

louis-bompart
Copy link
Collaborator

@louis-bompart louis-bompart commented Jul 31, 2019 •
edited
Loading

Waiting for coveo/search-ui#1170

Goal

Implement a result component that indicate when a result have been seen by an user (identified when sending the query).

Allow basic customization:

  • Hide/Show the icon
  • Change the label

Out of scopes, improvement for future PRs

  • Action on click
  • More info on hover.

Screenshots:

image
image

@louis-bompart louis-bompart marked this pull request as ready for review August 5, 2019 17:12
erocheleau
erocheleau requested changes Aug 6, 2019
View reviewed changes
Copy link
Collaborator

@erocheleau erocheleau left a comment

Choose a reason for hiding this comment

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

Very clean, love it

pages/index.html Outdated
@@ -44,107 +44,93 @@
</script>
</head>

<body id="search" class="CoveoSearchInterface" data-enable-history="true" style="padding: 1em;">
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 padding gone?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You didn't answer...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I gave you a general answer to your question about your file in a previous comment:
This file is only used for testing purposes. I based my template on the one from the JSUI and cleaned it because there was a main-section in another main section + duplicate code and all.

I don't think there's real value to be gained to further modify it because if you need it for development you're bound to customize it further.

Also my previous comment apply + the one of Analytic:

As I said, it's based on the Search page template of the coveo/search-ui.
https://github.com/coveo/search-ui/blob/master/pages/All.html I invite to check it out, you won't find an Analytics.

pages/index.html Outdated
@@ -44,107 +44,93 @@
</script>
</head>

<body id="search" class="CoveoSearchInterface" data-enable-history="true" style="padding: 1em;">
<span class="CoveoAnalytics"></span>
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 Analytics gone?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I said, it's based on the Search page template of the coveo/search-ui.
https://github.com/coveo/search-ui/blob/master/pages/All.html I invite to check it out, you won't find an Analytics.

pages/index.html Outdated Show resolved Hide resolved
pages/index.html Outdated
</div>
</div>
<div class="CoveoHiddenQuery"></div>

Copy link
Collaborator

Choose a reason for hiding this comment

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

blank line?

pages/index.html Outdated Show resolved Hide resolved
src/components/ViewedByCustomer/ViewedByCustomer.ts Outdated Show resolved Hide resolved
src/components/ViewedByCustomer/ViewedByCustomer.ts Outdated Show resolved Hide resolved
tests/components/ViewedByCustomer/ViewedByCustomer.spec.ts Outdated Show resolved Hide resolved
tests/components/ViewedByCustomer/ViewedByCustomer.spec.ts Outdated Show resolved Hide resolved
tests/components/ViewedByCustomer/ViewedByCustomer.spec.ts Outdated Show resolved Hide resolved
jeremierobert-coveo
jeremierobert-coveo requested changes Aug 6, 2019
View reviewed changes
Copy link
Collaborator

@jeremierobert-coveo jeremierobert-coveo left a comment

Choose a reason for hiding this comment

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

Vraiment nice, small diff really help to review especially review unit tests 😉 .

package.json Show resolved Hide resolved
src/components/ViewedByCustomer/ViewedByCustomer.ts Outdated Show resolved Hide resolved
src/components/ViewedByCustomer/ViewedByCustomer.ts Outdated Show resolved Hide resolved
src/components/ViewedByCustomer/ViewedByCustomer.ts Outdated Show resolved Hide resolved
src/components/ViewedByCustomer/ViewedByCustomer.ts Outdated Show resolved Hide resolved
tests/components/ViewedByCustomer/ViewedByCustomer.spec.ts Outdated Show resolved Hide resolved
tests/components/ViewedByCustomer/ViewedByCustomer.spec.ts Outdated Show resolved Hide resolved
tests/components/ViewedByCustomer/ViewedByCustomer.spec.ts Outdated Show resolved Hide resolved
tests/components/ViewedByCustomer/ViewedByCustomer.spec.ts Outdated Show resolved Hide resolved
tests/components/ViewedByCustomer/ViewedByCustomer.spec.ts Show resolved Hide resolved
@louis-bompart louis-bompart dismissed jeremierobert-coveo’s stale review August 6, 2019 18:53

We don't use em for font-size.

flapierre-coveo
flapierre-coveo approved these changes Aug 6, 2019
View reviewed changes
Copy link

@flapierre-coveo flapierre-coveo left a comment

Choose a reason for hiding this comment

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

Looks great!

src/components/ViewedByCustomer/ViewedByCustomer.ts Show resolved Hide resolved
tests/components/ViewedByCustomer/ViewedByCustomer.spec.ts Outdated Show resolved Hide resolved
tests/components/ViewedByCustomer/ViewedByCustomer.spec.ts Outdated Show resolved Hide resolved
src/components/ViewedByCustomer/ViewedByCustomer.ts Outdated Show resolved Hide resolved
tedre191
tedre191 approved these changes Aug 6, 2019
View reviewed changes
Copy link
Collaborator

@tedre191 tedre191 left a comment

Choose a reason for hiding this comment

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

Nice and clean, good job 👌

erocheleau
erocheleau previously requested changes Aug 7, 2019
View reviewed changes
Copy link
Collaborator

@erocheleau erocheleau left a comment

Choose a reason for hiding this comment

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

Answer the questions and it should be fine to merge.

pages/index.html Outdated Show resolved Hide resolved
pages/index.html Outdated Show resolved Hide resolved
@louis-bompart louis-bompart dismissed erocheleau’s stale review August 7, 2019 14:53

All changes to the file have been removed to reduce the complexity of the review

@louis-bompart louis-bompart merged commit aa25fe6 into coveo:master Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tedre191 tedre191 tedre191 approved these changes

@erocheleau erocheleau erocheleau approved these changes

@mikegron mikegron mikegron approved these changes

@jeremierobert-coveo jeremierobert-coveo jeremierobert-coveo approved these changes

@flapierre-coveo flapierre-coveo flapierre-coveo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@louis-bompart @tedre191 @erocheleau @mikegron @jeremierobert-coveo @flapierre-coveo