New feature proposal - Add link to opportunities #113

JuanMaRuiz · 2020-03-15T12:36:43Z

Hi @exterkamp! I was wondering if psi tool should show (on demand) some extra info as Page Speed Insights do.

Sometimes the information (or title) showed in the opportunities section is not enough. See issue #41.

So, here is my proposal. We could add the link to the opportunity so the user could navigate to the page from the terminal. If the user provides the option --links (no value needed) the link will be shown.

Here a few examples:

Default behaviour No --links options provide

With --links options
Here the display of the info looks different depending on the terminal. I have used terminal-link package. Here is the info about the support of this feature

Maybe the link could be shown in a different section an not as link of the opportunity field.

What do you think? Make sense for you this feature?

Best regards.

…re-optimizing-test

- Removed call to api. - Added test to options handler instead

- Merge branch 'feature-optimizing-test' of https://github.com/JuanMaRuiz/psi into feature-optimizing-test - Removed api-response-test

…iz/psi into feature-optimizing-test

Co-Authored-By: Shane Exterkamp <[email protected]>

…re dependency

* Update index file to use new api * Update googleapis dependency

* Add new option to show links to information about opportunities * Add test to cover new util that check the existence of url in a string

JuanMaRuiz · 2020-08-27T07:16:31Z

Hi @exterkamp any advance on it?

exterkamp

Review a long time coming, but overall this looks good! Just some small changes to make it air-tight!

cli.js

test/string-utils.test.js

exterkamp · 2020-08-27T16:42:54Z

lib/string-utils.js

@@ -0,0 +1,10 @@
+const getLink = str => {
+  const containsLink = str.match(/\(https?:(.*)\)/);


This seems like it would be more readable with a named group...something like

\((?<url>https?:.*)\)

and then it could be return containsLink.groups.url

So that there doesn't need to be any replacing of the "( )"s.

Much better. Many thanks. Done

Unfortunately named groups seem to be not supported in node 8 https://travis-ci.com/github/GoogleChromeLabs/psi/jobs/378505545.

Should I keep the original implementation due the support of this library for node 8? Any other option?

* Negative assertion on string-utils.test. * More readable regexp with named groups. * Fix typo in cli.js help

lib/output.js

exterkamp

These changes look good to me. But I think node 8 is still failing, or is stuck on a stale build. So once that is cleared up this is good to go!

exterkamp · 2020-09-01T18:43:31Z

lib/string-utils.js

@@ -0,0 +1,9 @@
+const getLink = str => {
+  const containsLink = str.match(/\((?<url>https?:.*)\)/);


Argh. Node 8 doesn't support named capture groups ☹️

We should go back to numbered groups then I suppose.

Yep 😢 it's not supported in node v8.

I have reverted the changes. Let me know if you prefer another approach.

JuanMaRuiz and others added 30 commits October 26, 2019 17:11

Added nyc for testint coverage

39472ab

Added index test

1f65651

Added test to check handleOpts method in index file

e95fe41

Merge branch 'master' of https://github.com/JuanMaRuiz/psi into featu…

da5adc1

…re-optimizing-test

Tests improvements

57fd768

- Removed call to api. - Added test to options handler instead

Added test for threshold

f8e7cc7

Updated some npm dependencies

21a1561

Optimizing unit tests

ffb8741

- Merge branch 'feature-optimizing-test' of https://github.com/JuanMaRuiz/psi into feature-optimizing-test - Removed api-response-test

Removed commented tests

695a877

Merge branch 'feature-optimizing-test' of https://github.com/JuanMaRu…

1930431

…iz/psi into feature-optimizing-test

Update test/index.test.js

a05bdfa

Co-Authored-By: Shane Exterkamp <[email protected]>

Update test/output.test.js

6a37baa

Co-Authored-By: Shane Exterkamp <[email protected]>

Reverted api-response.test

862aa58

Reverted googleapis npm dependency to previous verison

6ed67cb

Added proposed change of test definition

94fe536

Revert unnecessary change

8e8ab81

Moved optionsHandler and getTreshold to options-handler. Removed rewi…

22c9e77

…re dependency

Moved getReporter to options-handler

3b56a1a

Merge branch 'master' of https://github.com/GoogleChromeLabs/psi

dbda724

Merge branch 'master' of https://github.com/GoogleChromeLabs/psi

31bbc65

Merge branch 'master' of https://github.com/JuanMaRuiz/psi

d546d6c

Merge branch 'master' of https://github.com/GoogleChromeLabs/psi

31473da

Update googlepais to v40.0.0

4c65857

* Update index file to use new api * Update googleapis dependency

Merge branch 'master' of https://github.com/GoogleChromeLabs/psi

f0abcc6

Add new showLinks options

df6290b

* Add new option to show links to information about opportunities * Add test to cover new util that check the existence of url in a string

Merge branch 'master' of https://github.com/GoogleChromeLabs/psi

54dd8ea

Merge branch 'master' into add-link-to-opportunity-title

4d30f5f

Revert index file to match with master

b4a627e

Merge branch 'master' of https://github.com/JuanMaRuiz/psi

4668585

Merge branch 'master' into add-link-to-opportunity-title

9ef7f58

JuanMaRuiz changed the title ~~New feature proposal - Add link to opportunities~~ New feature proposal - Add link to opportunities :enhancement Mar 15, 2020

JuanMaRuiz changed the title ~~New feature proposal - Add link to opportunities :enhancement~~ New feature proposal - Add link to opportunities Mar 15, 2020

JuanMaRuiz added 2 commits March 16, 2020 21:43

Merge branch 'master' of https://github.com/GoogleChromeLabs/psi

2204ebf

Merge branch 'master' into add-link-to-opportunity-title

116d867

exterkamp self-assigned this Mar 16, 2020

exterkamp self-requested a review March 16, 2020 20:47

Add missed dependency after merge

4bb63f9

exterkamp requested changes Aug 27, 2020

View reviewed changes

Add improvements from code review

6d38bed

* Negative assertion on string-utils.test. * More readable regexp with named groups. * Fix typo in cli.js help

exterkamp reviewed Aug 27, 2020

View reviewed changes

lib/output.js Show resolved Hide resolved

Fix getTitle method to manage properly titles without links

69e0ca6

exterkamp approved these changes Aug 31, 2020

View reviewed changes

exterkamp reviewed Sep 1, 2020

View reviewed changes

JuanMaRuiz added 2 commits September 2, 2020 20:52

Revert named groups in regexp due is not supported by node v8

2850571

Add new param to README

e8edd2c

exterkamp merged commit f59a309 into GoogleChromeLabs:master Sep 3, 2020

JuanMaRuiz deleted the add-link-to-opportunity-title branch September 4, 2020 03:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New feature proposal - Add link to opportunities #113

New feature proposal - Add link to opportunities #113

JuanMaRuiz commented Mar 15, 2020 •

edited

Loading

JuanMaRuiz commented Aug 27, 2020

exterkamp left a comment

exterkamp Aug 27, 2020

JuanMaRuiz Aug 27, 2020 •

edited

Loading

exterkamp left a comment

exterkamp Sep 1, 2020 •

edited

Loading

JuanMaRuiz Sep 2, 2020

		@@ -0,0 +1,10 @@
		const getLink = str => {
		const containsLink = str.match(/\(https?:(.*)\)/);

		@@ -0,0 +1,9 @@
		const getLink = str => {
		const containsLink = str.match(/\((?<url>https?:.*)\)/);

New feature proposal - Add link to opportunities #113

New feature proposal - Add link to opportunities #113

Conversation

JuanMaRuiz commented Mar 15, 2020 • edited Loading

JuanMaRuiz commented Aug 27, 2020

exterkamp left a comment

Choose a reason for hiding this comment

exterkamp Aug 27, 2020

Choose a reason for hiding this comment

JuanMaRuiz Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

exterkamp left a comment

Choose a reason for hiding this comment

exterkamp Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

JuanMaRuiz Sep 2, 2020

Choose a reason for hiding this comment

JuanMaRuiz commented Mar 15, 2020 •

edited

Loading

JuanMaRuiz Aug 27, 2020 •

edited

Loading

exterkamp Sep 1, 2020 •

edited

Loading