Linked 2 components #446

pmongeau174 · 2017-05-04T18:54:25Z

All the rendering for the resultLink is now done by the renderUri function, just like the PrintableUri. ResultLink initiates the render instead of PrintableUri. Since PrintableUri already has a renderUri function, it is the one that is used for rendering. Need to add code to handle added options from ResultLink.
https://coveord.atlassian.net/browse/JSUI-1536

This change is

All the rendering for the resultLink is now done by the renderUri function, just like the PrintableUri. ResultLink initiates the render instead of PrintableUri. Since PrintableUri already has a renderUri function, it is the one that is used for rendering. Need to add code to handle added options from ResultLink. https://coveord.atlassian.net/browse/JSUI-1536

pmongeau174 · 2017-05-04T18:56:12Z

Not sure if PrintableUri needs to implement iComponentBindings. Also not sure if it's OK to always initiate the rendering from ResultLink.

https://coveord.atlassian.net/browse/JSUI-1536

coveralls · 2017-05-04T19:35:10Z

Coverage increased (+0.08%) to 80.783% when pulling 1319916 on JSUI-1536 into f9bc600 on master.

https://coveord.atlassian.net/browse/JSUI-1536

coveralls · 2017-05-04T19:56:43Z

Coverage increased (+0.09%) to 80.79% when pulling 12c4ab5 on JSUI-1536 into f9bc600 on master.

https://coveord.atlassian.net/browse/JSUI-1536

coveralls · 2017-05-04T20:14:56Z

Coverage increased (+0.01%) to 80.713% when pulling 4c1bc65 on JSUI-1536 into f9bc600 on master.

https://coveord.atlassian.net/browse/JSUI-1536

This fixed the issue where a PrintableUri with the AlwaysOpenInNewWindow option set to true would open the link in a new tab AND open the link in the current tab. https://coveord.atlassian.net/browse/JSUI-1536

https://coveord.atlassian.net/browse/JSUI-1536

results containing XML and results that don't now both use the same class for css. https://coveord.atlassian.net/browse/JSUI-1536

coveralls · 2017-05-12T12:50:37Z

Coverage increased (+0.5%) to 81.372% when pulling 5c9052e on JSUI-1536 into 5210fd2 on master.

coveralls · 2017-05-12T12:55:51Z

Coverage increased (+0.5%) to 81.372% when pulling 5c9052e on JSUI-1536 into 5210fd2 on master.

https://coveord.atlassian.net/browse/JSUI-1536

coveralls · 2017-05-15T15:03:51Z

Coverage increased (+0.4%) to 81.286% when pulling ca50bb6 on JSUI-1536 into f11678a on master.

olamothe · 2017-05-15T16:48:17Z

sass/_PrintableUri.scss

@@ -1,21 +1,33 @@
 @import "Variables";

-.CoveoPrintableUri{
+.CoveoPrintableUri {
+  justify-content:flex-start;


Use bourbon mixin.

@include justify-content(flex-start);

olamothe · 2017-05-15T16:48:19Z

sass/_PrintableUri.scss

+  font-size: $font-size-smallest;
+  @include clickable();
+  padding: 0 2px 0 2px;
+  display: flex;


Same, use bourbon mixin.

Basically, use bourbon mixin for every "flex" rules you are using.

Using bourbon mixin on every flex rule, but I noticed that the ones for the flex-box have a deprecation warning. Leave them as is or use bourbon for that too ?

olamothe · 2017-05-15T16:48:40Z

sass/_PrintableUri.scss

+  .coveo-printable-uri-part {
+    margin: 1px;
+    //box-sizing: border-box;


remove comments.

olamothe · 2017-05-15T16:50:03Z

sass/_PrintableUri.scss

+    overflow: hidden;
+    text-overflow: ellipsis;
+    //display:inline-block;


olamothe · 2017-05-15T16:54:02Z

src/ui/PrintableUri/PrintableUri.ts

+        this.uri = result.clickUri;
+        let stringAndHoles: StringAndHoles;
+        if (result.printableUri.indexOf('\\') == -1) {
+          stringAndHoles = StringAndHoles.shortenUri(result.printableUri, $$(element).width() / 7);


What's the / 7 purpose here ?

I understand it was there before, and you mostly imported the old implementation, but I would like if we had an explanation for this magic number/ a comment that explains why we are dividing by 7 like that.

olamothe · 2017-05-15T16:55:26Z

src/ui/PrintableUri/PrintableUri.ts

+          stringAndHoles = StringAndHoles.shortenPath(result.printableUri, $$(element).width() / 7);
+        }
+        this.shortenedUri = HighlightUtils.highlightString(stringAndHoles.value, result.printableUriHighlights, stringAndHoles.holes, 'coveo-highlight');
+        let link = $$('div');


for this whole component, you can replace most of the let by const. Only when you need to re-assign a variable should you need to use const. Once again, I know it was like that before, but we should always try to update/upgrade the codebase over time to use const when we can.

olamothe · 2017-05-15T16:57:00Z

src/ui/PrintableUri/PrintableUri.ts

+        }
+        this.shortenedUri = HighlightUtils.highlightString(stringAndHoles.value, result.printableUriHighlights, stringAndHoles.holes, 'coveo-highlight');
+        let link = $$('div');
+        link.setAttribute('title', result.printableUri);


this can all be replaced with

$$('div', {
className : 'coveo-printable-uri-part',
href : result.clickUri,
title : result.printableUri
}

This has not been changed :D

It should be modified to use the short hand method, instead of setting every attribute one by one.

olamothe · 2017-05-15T17:01:42Z

test/ui/PrintableUriTest.ts

+
+      it('should not modify the href template if there are no field specified', () => {
+        let hrefTemplate = 'test';
+        test = Mock.optionsResultComponentSetup<PrintableUri, IResultLinkOptions>(PrintableUri, { hrefTemplate: hrefTemplate }, fakeResult);


IPrintableUriOptions

Dominionized · 2017-05-16T17:01:48Z

src/ui/PrintableUri/PrintableUri.ts

    let seperator = document.createElement('span');
-    seperator.innerText = '>';
+    seperator.innerText = ' > ';


innerText is non-standard. You should use textContent instead.

Dominionized · 2017-05-16T17:03:57Z

src/ui/PrintableUri/PrintableUri.ts

-    let link = document.createElement('a');
-    this.bindLogOpenDocument(link);
-    link.href = uri;
+    let link = document.createElement('span');


You can create elements in a more concise way using our $$ utilty function:

const link = $$('span', { className: 'coveo-printable-uri-part' }, modifiedName);

Dominionized · 2017-05-16T17:06:33Z

test/ui/PrintableUriTest.ts

+    });
+
+    it('should display the XML correctly if the result has a very long parents field', () => {
+      fakeResult.raw.parents = '<?xml version="1.0" encoding="utf-16"?><parents><parent name="Organization" uri="https://na17.salesforce.com/home/home.jsp" /><parent name="Technical_Article__ka" uri="http://www.salesforce.com/org:organization/articletype:Technical_Article" /><parent name="Generator became sentient and refuses to cut power" uri="https://na17.salesforce.com/kA0o00000003Wpk" /><parent name="un autre test" uri="http//:google.ca" /></parents>';


Just a suggestion : you can use template strings using a backtick (`) to write multi-line strings. It makes the code look a bit better.

Dominionized · 2017-05-16T17:07:24Z

test/ui/ResultLinkTest.ts

@@ -63,7 +63,7 @@ export function ResultLinkTest() {
        expect(window.open).toHaveBeenCalledWith(hrefTemplate, jasmine.anything());
      });

-      it('should replaces fields in the href template by the results equivalent', () => {
+      it('should replace fields in the href template by the results equivalent', () => {


It should replaces hobbitses 😛

https://coveord.atlassian.net/browse/JSUI-1536

coveralls · 2017-05-18T13:32:13Z

Coverage increased (+1.3%) to 82.14% when pulling 103a4fe on JSUI-1536 into f11678a on master.

https://coveord.atlassian.net/browse/JSUI-1536

coveralls · 2017-05-18T14:07:09Z

Coverage increased (+1.3%) to 82.14% when pulling 974a5da on JSUI-1536 into f11678a on master.

https://coveord.atlassian.net/browse/JSUI-1536

coveralls · 2017-05-23T14:11:20Z

Coverage increased (+0.4%) to 82.199% when pulling b09f24b on JSUI-1536 into c5b7caa on master.

olamothe · 2017-05-23T15:05:49Z

src/ui/PrintableUri/PrintableUri.ts

@@ -17,7 +17,9 @@ export interface IPrintableUriOptions extends IResultLinkOptions {
 }

 /**
- * The PrintableUri component displays the URI, or path, to access a result.
+ * The PrintableUri component inherits from the ResultLink component and supports all of its options.


We should create a link in the documentation ( {@link ResultLink} )

@fbeaudoincoveo Any comments to add on this ?

I think we should

I would also put back-ticks around PrintableUri (PrintableUri). And [ResultLink]{@link ResultLink} instead of just {@link ResultLink}. Otherwise, it's great :)

olamothe · 2017-05-23T15:06:40Z

src/ui/PrintableUri/PrintableUri.ts

+        }
+        this.shortenedUri = HighlightUtils.highlightString(stringAndHoles.value, result.printableUriHighlights, stringAndHoles.holes, 'coveo-highlight');
+        let link = $$('div');
+        link.setAttribute('title', result.printableUri);


This has not been changed :D

It should be modified to use the short hand method, instead of setting every attribute one by one.

olamothe · 2017-05-23T15:07:22Z

src/ui/PrintableUri/PrintableUri.ts

        this.element.innerHTML = newTitle ? StreamHighlightUtils.highlightStreamText(newTitle, this.result.termsToHighlight, this.result.phrasesToHighlight) : this.result.clickUri;
      }
    }
    element.title = this.result.printableUri;
  }

  public buildSeparator() {
-    let seperator = document.createElement('span');
-    seperator.innerText = ' > ';
+    const seperator = document.createElement('span');


This should use the short hand method also with $$('span', {className : 'coveo-rpintable-uri-separator'}, ' > ');

https://coveord.atlassian.net/browse/JSUI-1536

coveralls · 2017-05-23T17:13:59Z

Coverage increased (+0.4%) to 82.228% when pulling 734d93e on JSUI-1536 into c5b7caa on master.

coveralls · 2017-05-23T17:18:41Z

Coverage increased (+0.4%) to 82.228% when pulling 734d93e on JSUI-1536 into c5b7caa on master.

https://coveord.atlassian.net/browse/JSUI-1536

coveralls · 2017-05-23T17:42:34Z

Coverage increased (+0.4%) to 82.225% when pulling 21c5370 on JSUI-1536 into 742d229 on master.

coveralls · 2017-05-23T17:44:02Z

Coverage increased (+0.4%) to 82.225% when pulling 21c5370 on JSUI-1536 into 742d229 on master.

ResultLink options imported into PrintableUri options.

1319916

https://coveord.atlassian.net/browse/JSUI-1536

pmongeau174 requested review from olamothe and dms1lva May 4, 2017 19:30

pmongeau174 self-assigned this May 4, 2017

Imported options into child class

12c4ab5

https://coveord.atlassian.net/browse/JSUI-1536

Small mistake: used " " instead of ' ' for import path.

4c1bc65

https://coveord.atlassian.net/browse/JSUI-1536

pmongeau174 added 13 commits May 4, 2017 16:26

' ' instead of " " for imports.

80a9ed8

https://coveord.atlassian.net/browse/JSUI-1536

Merge remote-tracking branch 'origin/JSUI-1536' into JSUI-1536

f389c38

https://coveord.atlassian.net/browse/JSUI-1536

Resolve conflict

3758a1f

https://coveord.atlassian.net/browse/JSUI-1536

Fixed CSS styling, imported _ from underscore, fixed PrintableUri render

2a9320b

https://coveord.atlassian.net/browse/JSUI-1536

Fixed typos in ResultLink tests, created tests for PrintableUri

8ec7b27

https://coveord.atlassian.net/browse/JSUI-1536

Removed unused test

b08a551

https://coveord.atlassian.net/browse/JSUI-1536

Put the link in a 'div' instead of 'a'.

eab4aef

This fixed the issue where a PrintableUri with the AlwaysOpenInNewWindow option set to true would open the link in a new tab AND open the link in the current tab. https://coveord.atlassian.net/browse/JSUI-1536

Switched 'a' for 'div' to ensure tests work

da62d70

https://coveord.atlassian.net/browse/JSUI-1536

Tests OK, figuring out flexbox

aeacc97

https://coveord.atlassian.net/browse/JSUI-1536

Adjusted CSS to get desired effect. Still needs a bit of tuning.

22c3207

https://coveord.atlassian.net/browse/JSUI-1536

Set every printableUri's html title to it's printableUri

163c3af

https://coveord.atlassian.net/browse/JSUI-1536

Same class for both rendering types

7a32e7f

results containing XML and results that don't now both use the same class for css. https://coveord.atlassian.net/browse/JSUI-1536

Merge branch 'master' into JSUI-1536

5c9052e

pmongeau174 added 4 commits May 12, 2017 08:57

Replaced "" with ''

eb41169

https://coveord.atlassian.net/browse/JSUI-1536

Merge remote-tracking branch 'origin/JSUI-1536' into JSUI-1536

dc5fdc6

Misplaced else and prettify run

be47c82

https://coveord.atlassian.net/browse/JSUI-1536

Merge branch 'master' into JSUI-1536

ca50bb6

olamothe requested changes May 15, 2017

View reviewed changes

Dominionized reviewed May 16, 2017

View reviewed changes

Changes to comply to comments

103a4fe

https://coveord.atlassian.net/browse/JSUI-1536

Fixed error in build

974a5da

https://coveord.atlassian.net/browse/JSUI-1536

pmongeau174 added 2 commits May 19, 2017 17:02

HTMLElement not needed

411a4e8

https://coveord.atlassian.net/browse/JSUI-1536

Merge branch 'master' into JSUI-1536

b09f24b

olamothe requested changes May 23, 2017

View reviewed changes

pmongeau174 added 2 commits May 23, 2017 13:03

Comment suggestions applied

5eac472

https://coveord.atlassian.net/browse/JSUI-1536

Forgot backticks. Oops.

734d93e

https://coveord.atlassian.net/browse/JSUI-1536

pmongeau174 added 2 commits May 23, 2017 13:31

One last change, shorter syntax

05c18bf

https://coveord.atlassian.net/browse/JSUI-1536

Merge branch 'master' into JSUI-1536

21c5370

pmongeau174 merged commit 9a5b8eb into master May 23, 2017

pmongeau174 deleted the JSUI-1536 branch May 23, 2017 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linked 2 components #446

Linked 2 components #446

pmongeau174 commented May 4, 2017 •

edited by olamothe

Loading

pmongeau174 commented May 4, 2017

coveralls commented May 4, 2017

coveralls commented May 4, 2017

coveralls commented May 4, 2017

coveralls commented May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 15, 2017

olamothe May 15, 2017

olamothe May 15, 2017

pmongeau174 May 17, 2017

olamothe May 15, 2017

olamothe May 15, 2017

olamothe May 15, 2017

olamothe May 15, 2017

olamothe May 15, 2017

olamothe May 23, 2017

olamothe May 15, 2017

Dominionized May 16, 2017

Dominionized May 16, 2017

Dominionized May 16, 2017

Dominionized May 16, 2017

coveralls commented May 18, 2017

coveralls commented May 18, 2017

coveralls commented May 23, 2017

olamothe May 23, 2017

fbeaudoincoveo May 23, 2017

olamothe May 23, 2017

olamothe May 23, 2017

coveralls commented May 23, 2017

coveralls commented May 23, 2017

coveralls commented May 23, 2017

coveralls commented May 23, 2017

Linked 2 components #446

Linked 2 components #446

Conversation

pmongeau174 commented May 4, 2017 • edited by olamothe Loading

pmongeau174 commented May 4, 2017

coveralls commented May 4, 2017

coveralls commented May 4, 2017

coveralls commented May 4, 2017

coveralls commented May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 18, 2017

coveralls commented May 18, 2017

coveralls commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 23, 2017

coveralls commented May 23, 2017

coveralls commented May 23, 2017

coveralls commented May 23, 2017

pmongeau174 commented May 4, 2017 •

edited by olamothe

Loading