accept query parameters (for tags and categories) #28

AmarOk1412 · 2016-10-10T17:59:20Z

Cf. issue #25

Add 3 parameters. Now we can get http://firefox-dev.tools/?easy=true&mentored=true&category=inspector for example.

nt1m · 2016-10-10T18:45:22Z

app.js

@@ -147,6 +170,10 @@ function createToolListMarkup(parentEl) {
      }
    });

+    if(categorieInUrl === keys[i]) {


nits: space after if

nt1m · 2016-10-10T18:45:40Z

app.js

@@ -134,6 +156,7 @@ function getToolTooltip(id) {

 function createToolListMarkup(parentEl) {
  var keys = Object.keys(COMPONENT_MAPPING);
+  var categorieInUrl = getParameterByName("category");


nit: categoryInUrl

nt1m · 2016-10-10T18:46:17Z

app.js

@@ -13,6 +13,28 @@ function hasFilter(name, filters) {
  return false;
 }

+function getParameterByName(name, url) {
+  if (!url) url = window.location.href;


I would prefer getting the parameters from window.location.search rather than href.

nt1m

Looks good so far! Thanks for the PR

nt1m · 2016-10-10T18:54:19Z

app.js

+}
+
+function parseTagsInUrl(url) {
+  if (!url) url = window.location.search;


Can you remove this line and the url parameter in this function?

Woops, my bad

nt1m · 2016-10-10T18:55:45Z

app.js

@@ -13,6 +13,28 @@ function hasFilter(name, filters) {
  return false;
 }

+function getParameterByName(name, url) {


The url parameter is unused in the file, can you remove it?

nt1m · 2016-10-10T18:57:43Z

app.js

@@ -13,6 +13,28 @@ function hasFilter(name, filters) {
  return false;
 }

+function getParameterByName(name, url) {
+  if (!url) url = window.location.search;


var urlParams = window.location.search;

nt1m · 2016-10-10T18:58:30Z

app.js

@@ -13,6 +13,28 @@ function hasFilter(name, filters) {
  return false;
 }

+function getParameterByName(name, url) {
+  if (!url) url = window.location.search;
+  name = name.replace(/[\[\]]/g, "\\$&");


nit: add a new line after this line

nt1m · 2016-10-10T18:59:25Z

app.js

+  if (!url) url = window.location.search;
+  name = name.replace(/[\[\]]/g, "\\$&");
+  var regex = new RegExp("[?&]" + name + "(=([^&#]*)|&|#|$)"),
+  results = regex.exec(url);


nit: use var results (we don't use the var x, y; notation)
and add a new line after this one.

nt1m · 2016-10-11T09:17:54Z

I'll let @captainbrosset do a last review.

captainbrosset

Thanks for the PR.
Here are a few more comments.

captainbrosset · 2016-10-11T10:01:52Z

app.js

+  var urlParams = window.location.search;
+  name = name.replace(/[\[\]]/g, "\\$&");
+
+  var results = new RegExp("[?&]" + name + "(=([^&#]*)|&|#|$)").exec(urlParams);


No need for parsing url parameters, the URL constructor can be used instead:

var params = new URL(window.location).searchParams;

This gives you a ready to use object to retrieve the parameters. See https://developer.mozilla.org/en-US/docs/Web/API/URL

It seems to have decent browser support too except for Edge/IE, but I've never tested the site in these browsers.

Awesome! It's a lot better than a Regex

captainbrosset · 2016-10-11T10:06:13Z

app.js

@@ -134,6 +156,7 @@ function getToolTooltip(id) {

 function createToolListMarkup(parentEl) {
  var keys = Object.keys(COMPONENT_MAPPING);
+  var categoryInUrl = getParameterByName("category");


I would rename category to tool. We don't use the word category anywhere in the code or the UI, and tool seems to me like a better fit. Also easier to type.

captainbrosset · 2016-10-11T10:08:25Z

app.js

+  return decodeURIComponent(results[2].replace(/\+/g, " "));
+}
+
+function parseTagsInUrl() {


This function doesn't parse tags. It retrieves them form the URL using another function and then sets the corresponding inputs.
So I suggest changing its name to setFiltersFromUrlParams.

captainbrosset · 2016-10-11T10:16:44Z

app.js

+function parseTagsInUrl() {
+  var easy = getParameterByName("easy");
+  var mentored = getParameterByName("mentored");
+  if (easy === "false" || (mentored === "true" && easy === null)) {


About this logic: why do we need to even have values for easy and mentored.
It seems to me that this could work with just ?easy&mentored, or ?easy, or ?mentored.
I don't think we need them to have true/false as a value. It makes the URL harder to type, and we can derive the same meaning with the values.

So:

if neither easy nor mentored are part of the URL params, then we use to the default settings,

if easy is in the URL params but not mentored, then we search for easy/non-mentored bugs

if easy is not in the URL params but mentored is, then we search for non-easy/mentored bugs

if both easy and mentored are in the URL params, then we search for easy/mentored bugs

How does that sound?

I agree, it's easier to write.

nt1m · 2016-10-13T23:28:58Z

app.js

+  }
+
+  if (params.has(name)) {
+    return '';


Nit: use double quotes

captainbrosset

Thanks for the new commits. Just a few final nit comments, I'm not blocking the review just for this, but if you do have time, it'd gladly take another commit with these fixed.

captainbrosset · 2016-10-14T07:42:38Z

app.js

@@ -303,7 +342,7 @@ function matchesSearchString(bug) {
  var query = searchString.toLowerCase();

  return bug.summary.toLowerCase().indexOf(query) !== -1 ||
-         (bug.id + "").indexOf(query) !== -1;
+  (bug.id + "").indexOf(query) !== -1;


nit: this seems like an unrelated formatting change. Can you change it back?

captainbrosset · 2016-10-14T07:42:43Z

app.js

@@ -280,7 +319,7 @@ function createBugMarkup(bug) {
      attributes: {
        "class": "tag old-bug",
        "title": "This bug has been inactive for more than " +
-                 INACTIVE_AFTER + " days"
+        INACTIVE_AFTER + " days"


nit: this seems like an unrelated formatting change. Can you change it back?

captainbrosset · 2016-10-14T07:43:00Z

app.js

+    "component",
+    "keywords",
+    "mentors",
+    "attachments"],


nit: this seems like an unrelated formatting change. Can you change it back?

captainbrosset · 2016-10-14T12:41:32Z

Thanks @AmarOk1412

accept query parameters (for tags and categories)

faef3f9

nt1m reviewed Oct 10, 2016

View reviewed changes

app.js

@@ -147,6 +170,10 @@ function createToolListMarkup(parentEl) {

}

});

if(categorieInUrl === keys[i]) {

Copy link

nt1m Oct 10, 2016

Choose a reason for hiding this comment

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

nits: space after if

nt1m reviewed Oct 10, 2016

View reviewed changes

typo

0beee9e

nt1m suggested changes Oct 10, 2016

View reviewed changes

remove useless variables

eec0607

captainbrosset suggested changes Oct 11, 2016

View reviewed changes

clean code and remove '=true' in URL for easy & mentored

d381202

nt1m reviewed Oct 13, 2016

View reviewed changes

app.js

}

if (params.has(name)) {

return '';

Copy link

nt1m Oct 13, 2016

Choose a reason for hiding this comment

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

Nit: use double quotes

replace single by double quotes

6d75238

captainbrosset approved these changes Oct 14, 2016

View reviewed changes

format code

5a791ae

captainbrosset merged commit e2dab05 into firefox-devtools:gh-pages Oct 14, 2016

captainbrosset mentioned this pull request Oct 14, 2016

Accept query parameters to automatically start search #25

Closed

AmarOk1412 deleted the query_param branch October 14, 2016 12:43

nchevobbe mentioned this pull request Oct 20, 2016

Append query parameter to URL when interacting with UI #30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

accept query parameters (for tags and categories) #28

accept query parameters (for tags and categories) #28

AmarOk1412 commented Oct 10, 2016

nt1m Oct 10, 2016

nt1m Oct 10, 2016

nt1m Oct 10, 2016

nt1m left a comment

nt1m Oct 10, 2016

AmarOk1412 Oct 10, 2016

nt1m Oct 10, 2016

nt1m Oct 10, 2016

nt1m Oct 10, 2016

nt1m Oct 10, 2016 •

edited

nt1m commented Oct 11, 2016

captainbrosset left a comment

captainbrosset Oct 11, 2016 •

edited

AmarOk1412 Oct 13, 2016

captainbrosset Oct 11, 2016

captainbrosset Oct 11, 2016

captainbrosset Oct 11, 2016

AmarOk1412 Oct 13, 2016

nt1m Oct 13, 2016

captainbrosset left a comment

captainbrosset Oct 14, 2016

captainbrosset Oct 14, 2016

captainbrosset Oct 14, 2016

captainbrosset commented Oct 14, 2016

accept query parameters (for tags and categories) #28

accept query parameters (for tags and categories) #28

Conversation

AmarOk1412 commented Oct 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nt1m left a 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

nt1m Oct 10, 2016 • edited

Choose a reason for hiding this comment

nt1m commented Oct 11, 2016

captainbrosset left a comment

Choose a reason for hiding this comment

captainbrosset Oct 11, 2016 • edited

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

captainbrosset left a 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

captainbrosset commented Oct 14, 2016

nt1m Oct 10, 2016 •

edited

captainbrosset Oct 11, 2016 •

edited