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

Version 3 #206

Merged
merged 37 commits into from
Sep 11, 2018
Merged

Version 3 #206

merged 37 commits into from
Sep 11, 2018

Conversation

th0r
Copy link
Collaborator

@th0r th0r commented Aug 28, 2018

@th0r th0r changed the title [WIP] Implement modules search [WIP] Version 3 Aug 28, 2018
@valscion
Copy link
Member

Exciting! Let me know if you need help in testing or want comments for anything ☺️

@th0r
Copy link
Collaborator Author

th0r commented Aug 29, 2018

@valscion thanks! It will be much easier to develop new features with MobX as state management solution. I would be grateful if you could test search UI and share some thoughts.

@valscion

This comment has been minimized.

@valscion

This comment has been minimized.

@th0r
Copy link
Collaborator Author

th0r commented Aug 29, 2018

Hmm seems like npm run start after npm install isn't enough to get a working version.

Yep, I see the same on CI but I don't understand why it happens - it works locally for me. Do you have client/lib/PureComponent.jsx file?

@valscion

This comment has been minimized.

@th0r

This comment has been minimized.

@valscion

This comment has been minimized.

@th0r

This comment has been minimized.

@valscion

This comment has been minimized.

@valscion
Copy link
Member

I wonder if now would also be a good chance on somehow improve the discoverability of the sidebar? It's always been quite hidden in the UI, and it hasn't been instantly clear to me or our developers that the side bar even is there.

I wonder how it would look like if the sidebar was visible for, say, 1 second when you first open the page and then gets hidden with the animation? That way users might see that the left side might have some information there if you go check it out

@th0r
Copy link
Collaborator Author

th0r commented Aug 29, 2018

It wasn't immediately obvious to me which results were directories with more content in them and which were leaf nodes. I wonder if just adding a final forward slash for directories would be enough?

Yes, I thought about that actually. We can just add some icons for module/directory. I think it should solve this problem, right?

Another thing that I got a bit confused about was that I first thought the search would also filter the chunks below.

Hmm. Do you think it should filter chunks as well? Maybe it's better to add additional filtering input below "Show chunks:" title?

I wonder if now would also be a good chance on somehow improve the discoverability of the sidebar? It's always been quite hidden in the UI, and it hasn't been instantly clear to me or our developers that the side bar even is there.

I wonder how it would look like if the sidebar was visible for, say, 1 second when you first open the page and then gets hidden with the animation? That way users might see that the left side might have some information there if you go check it out

Actually, that's exactly how it works right now 😁

componentDidMount() {
    this.hideTimeoutId = setTimeout(() => this.toggleVisibility(false), 1500);
    this.hideContentTimeout = null;
  }

Maybe we should increase this timeout?

@valscion
Copy link
Member

Actually, that's exactly how it works right now 😁

Oh yeah, that's true — I think I've grown so accustomed to this behavior that I forget about it 😄

I might be a bit strange, but I keep my macOS Dock hidden in the left area of my screen, so it's always been a bit difficult for me to open the sidebar 😄

screen shot 2018-08-29 at 12 28 09

@valscion
Copy link
Member

Hmm. Do you think it should filter chunks as well? Maybe it's better to add additional filtering input below "Show chunks:" title?

Hmm maybe an additional filter would be enough? I think if it filtered chunks automatically, that could be confusing.

I'm not even sure if this needs any additional search for the chunks to be honest. It could be added later on as well if it feels like it's necessary. I didn't seem to need one as I was testing this out.

Yes, I thought about that actually. We can just add some icons for module/directory. I think it should solve this problem, right?

Yeah icons would work, too 👍

@valscion
Copy link
Member

The 400ms debounce is great 👍 feels much more responsive and I don't have to wait for too long to see the results.

@valscion
Copy link
Member

Really like the new pinning option! Now I can really drill into large packages and find where they're duplicated. Great work! 💯 👍

@valscion
Copy link
Member

After npm install, I got this diff:

diff --git a/package-lock.json b/package-lock.json
index 55432e2..76d289e 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -5432,7 +5432,7 @@
     },
     "http-proxy-middleware": {
       "version": "0.18.0",
-      "resolved": "http://registry.npmjs.org/http-proxy-middleware/-/http-proxy-middleware-0.18.0.tgz",
+      "resolved": "https://registry.npmjs.org/http-proxy-middleware/-/http-proxy-middleware-0.18.0.tgz",
       "integrity": "sha512-Fs25KVMPAIIcgjMZkVHJoKg9VcXcC1C8yb9JUgeDvVXY0S/zgVIhMb+qVswDIgtJe2DfckMSY2d6TuTEutlk6Q==",
       "dev": true,
       "requires": {

Seems like one resolved field is using http:// incorrectly?

@th0r
Copy link
Collaborator Author

th0r commented Sep 10, 2018

After npm install, I got this diff

Thanks, updated package-lock.

@valscion
Copy link
Member

Hmm now I get even more diff after running npm install:

diff --git a/package-lock.json b/package-lock.json
index 3048cd6..89e65f7 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1198,7 +1198,7 @@
       "dependencies": {
         "acorn": {
           "version": "3.3.0",
-          "resolved": "http://registry.npmjs.org/acorn/-/acorn-3.3.0.tgz",
+          "resolved": "https://registry.npmjs.org/acorn/-/acorn-3.3.0.tgz",
           "integrity": "sha1-ReN/s56No/JbruP/U2niu18iAXo=",
           "dev": true
         }
@@ -1627,7 +1627,7 @@
         },
         "chalk": {
           "version": "1.1.3",
-          "resolved": "http://registry.npmjs.org/chalk/-/chalk-1.1.3.tgz",
+          "resolved": "https://registry.npmjs.org/chalk/-/chalk-1.1.3.tgz",
           "integrity": "sha1-qBFcVeSnAv5NFQq9OHKCKn4J/Jg=",
           "dev": true,
           "requires": {
@@ -2013,7 +2013,7 @@
     },
     "buffer": {
       "version": "4.9.1",
-      "resolved": "http://registry.npmjs.org/buffer/-/buffer-4.9.1.tgz",
+      "resolved": "https://registry.npmjs.org/buffer/-/buffer-4.9.1.tgz",
       "integrity": "sha1-bRu2AbB6TvztlwlBMgkwJ8lbwpg=",
       "dev": true,
       "requires": {
@@ -3016,7 +3016,7 @@
       "dependencies": {
         "lodash": {
           "version": "3.0.1",
-          "resolved": "http://registry.npmjs.org/lodash/-/lodash-3.0.1.tgz",
+          "resolved": "https://registry.npmjs.org/lodash/-/lodash-3.0.1.tgz",
           "integrity": "sha1-FNSQKKOLx0AkHRHi7NV+wG1zwZo=",
           "dev": true
         }
@@ -3388,7 +3388,7 @@
       "dependencies": {
         "minimist": {
           "version": "1.2.0",
-          "resolved": "http://registry.npmjs.org/minimist/-/minimist-1.2.0.tgz",
+          "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.0.tgz",
           "integrity": "sha1-o1AIsg9BOD7sH7kU9M1d95omQoQ=",
           "dev": true
         },
@@ -3867,7 +3867,7 @@
     },
     "express": {
       "version": "4.16.3",
-      "resolved": "http://registry.npmjs.org/express/-/express-4.16.3.tgz",
+      "resolved": "https://registry.npmjs.org/express/-/express-4.16.3.tgz",
       "integrity": "sha1-avilAjUNsyRuzEvs9rWjTSL37VM=",
       "requires": {
         "accepts": "~1.3.5",
@@ -3931,7 +3931,7 @@
     },
     "external-editor": {
       "version": "2.2.0",
-      "resolved": "http://registry.npmjs.org/external-editor/-/external-editor-2.2.0.tgz",
+      "resolved": "https://registry.npmjs.org/external-editor/-/external-editor-2.2.0.tgz",
       "integrity": "sha512-bSn6gvGxKt+b7+6TKEv1ZycHleA7aHhRHyAqJyp5pbUFuYYNIzpZnQDk7AsYckyWdEnTeAnay0aCy2aV6iTk9A==",
       "dev": true,
       "requires": {
@@ -5392,7 +5392,7 @@
     },
     "http-errors": {
       "version": "1.6.3",
-      "resolved": "http://registry.npmjs.org/http-errors/-/http-errors-1.6.3.tgz",
+      "resolved": "https://registry.npmjs.org/http-errors/-/http-errors-1.6.3.tgz",
       "integrity": "sha1-i1VoC7S+KDoLW/TqLjhYC+HZMg0=",
       "requires": {
         "depd": "~1.1.2",
@@ -5420,7 +5420,7 @@
     },
     "http-proxy-middleware": {
       "version": "0.18.0",
-      "resolved": "http://registry.npmjs.org/http-proxy-middleware/-/http-proxy-middleware-0.18.0.tgz",
+      "resolved": "https://registry.npmjs.org/http-proxy-middleware/-/http-proxy-middleware-0.18.0.tgz",
       "integrity": "sha512-Fs25KVMPAIIcgjMZkVHJoKg9VcXcC1C8yb9JUgeDvVXY0S/zgVIhMb+qVswDIgtJe2DfckMSY2d6TuTEutlk6Q==",
       "dev": true,
       "requires": {
@@ -6096,7 +6096,7 @@
     },
     "jsonfile": {
       "version": "2.4.0",
-      "resolved": "http://registry.npmjs.org/jsonfile/-/jsonfile-2.4.0.tgz",
+      "resolved": "https://registry.npmjs.org/jsonfile/-/jsonfile-2.4.0.tgz",
       "integrity": "sha1-NzaitCi4e72gzIO1P6PWM6NcKug=",
       "dev": true,
       "requires": {
@@ -6481,7 +6481,7 @@
       "dependencies": {
         "minimist": {
           "version": "1.2.0",
-          "resolved": "http://registry.npmjs.org/minimist/-/minimist-1.2.0.tgz",
+          "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.0.tgz",
           "integrity": "sha1-o1AIsg9BOD7sH7kU9M1d95omQoQ=",
           "dev": true
         }
@@ -6575,7 +6575,7 @@
     },
     "minimist": {
       "version": "0.0.8",
-      "resolved": "http://registry.npmjs.org/minimist/-/minimist-0.0.8.tgz",
+      "resolved": "https://registry.npmjs.org/minimist/-/minimist-0.0.8.tgz",
       "integrity": "sha1-hX/Kv8M5fSYluCKCYuhqp6ARsF0="
     },
     "minstache": {
@@ -6589,7 +6589,7 @@
       "dependencies": {
         "commander": {
           "version": "1.0.4",
-          "resolved": "http://registry.npmjs.org/commander/-/commander-1.0.4.tgz",
+          "resolved": "https://registry.npmjs.org/commander/-/commander-1.0.4.tgz",
           "integrity": "sha1-Xt6xruI8T7VBprcNaSq+8ZZpotM=",
           "dev": true,
           "requires": {
@@ -6639,7 +6639,7 @@
     },
     "mkdirp": {
       "version": "0.5.1",
-      "resolved": "http://registry.npmjs.org/mkdirp/-/mkdirp-0.5.1.tgz",
+      "resolved": "https://registry.npmjs.org/mkdirp/-/mkdirp-0.5.1.tgz",
       "integrity": "sha1-MAV0OOrGz3+MR2fzhkjWaX11yQM=",
       "requires": {
         "minimist": "0.0.8"
@@ -6679,7 +6679,7 @@
       "dependencies": {
         "commander": {
           "version": "2.15.1",
-          "resolved": "http://registry.npmjs.org/commander/-/commander-2.15.1.tgz",
+          "resolved": "https://registry.npmjs.org/commander/-/commander-2.15.1.tgz",
           "integrity": "sha512-VlfT9F3V0v+jr4yxPc5gg9s62/fIVWsd2Bk2iD435um1NlGMYdVCq+MjcXnhYq2icNOizHr1kK+5TI6H0Hy0ag==",
           "dev": true
         },
@@ -6989,7 +6989,7 @@
       "dependencies": {
         "minimist": {
           "version": "1.2.0",
-          "resolved": "http://registry.npmjs.org/minimist/-/minimist-1.2.0.tgz",
+          "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.0.tgz",
           "integrity": "sha1-o1AIsg9BOD7sH7kU9M1d95omQoQ=",
           "dev": true
         }
@@ -7226,7 +7226,7 @@
     },
     "os-locale": {
       "version": "1.4.0",
-      "resolved": "http://registry.npmjs.org/os-locale/-/os-locale-1.4.0.tgz",
+      "resolved": "https://registry.npmjs.org/os-locale/-/os-locale-1.4.0.tgz",
       "integrity": "sha1-IPnxeuKe00XoveWDsT0gCYA8FNk=",
       "dev": true,
       "requires": {
@@ -8316,7 +8316,7 @@
       "dependencies": {
         "minimist": {
           "version": "1.2.0",
-          "resolved": "http://registry.npmjs.org/minimist/-/minimist-1.2.0.tgz",
+          "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.0.tgz",
           "integrity": "sha1-o1AIsg9BOD7sH7kU9M1d95omQoQ=",
           "dev": true
         }
@@ -9690,7 +9690,7 @@
     },
     "table": {
       "version": "4.0.3",
-      "resolved": "http://registry.npmjs.org/table/-/table-4.0.3.tgz",
+      "resolved": "https://registry.npmjs.org/table/-/table-4.0.3.tgz",
       "integrity": "sha512-S7rnFITmBH1EnyKcvxBh1LjYeQMmnZtCXSEbHcH6S0NoKit24ZuFO/T1vDcLdYsLQkM188PVVhQmzKIuThNkKg==",
       "dev": true,
       "requires": {
@@ -11115,7 +11115,7 @@
     },
     "wrap-ansi": {
       "version": "2.1.0",
-      "resolved": "http://registry.npmjs.org/wrap-ansi/-/wrap-ansi-2.1.0.tgz",
+      "resolved": "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-2.1.0.tgz",
       "integrity": "sha1-2Pw9KE3QV5T+hJc8rs3Rz4JP3YU=",
       "dev": true,
       "requires": {

I'm running node v8.9.4 and npm v6.1.0 currently. My npm config list outputs this:

; cli configs
metrics-registry = "https://registry.npmjs.org/"
scope = ""
user-agent = "npm/6.1.0 node/v8.9.4 darwin x64"

; userconfig /Users/vesa/.npmrc
init.author.email = "<redacted from issue comment>"
init.author.name = "Vesa Laakso"
init.license = "MIT"

; node bin location = /Users/vesa/.nvm/versions/node/v8.9.4/bin/node
; cwd = /Users/vesa/code/muut/webpack-bundle-analyzer/webpack-bundle-analyzer-plugin
; HOME = /Users/vesa
; "npm config ls -l" to show all defaults.

I wonder if this diff is due to some version difference between my machine and yours? I don't know, I haven't used package lock that much as we currently use Yarn at work.

@th0r
Copy link
Collaborator Author

th0r commented Sep 11, 2018

What is metrics-registry in your config?

@valscion
Copy link
Member

valscion commented Sep 11, 2018

https://docs.npmjs.com/misc/config#metrics-registry

The registry you want to send cli metrics to if send-metrics is true.

Haven't set it myself, could be that some tool had set it

@valscion
Copy link
Member

Love the new icons for the pin and the toggle button!

I'm not sure if the style on sidebar toggle button is working correctly for me?

GIF of hover effect

kapture 2018-09-11 at 10 47 15


Maybe it should have some sort of background when hovering instead of background being transparent?

@valscion
Copy link
Member

valscion commented Sep 11, 2018

Here's the full output of npm config list -l. The registry value has https:// for me, and I suppose that is why my installations always want the https:// part to be in the package lock:

; cli configs
long = true
metrics-registry = "https://registry.npmjs.org/"
scope = ""
user-agent = "npm/6.1.0 node/v8.9.4 darwin x64"

; userconfig /Users/vesa/.npmrc
init.author.email = "<redacted>"
init.author.name = "Vesa Laakso"
init.license = "MIT"

; default values
access = null
allow-same-version = false
also = null
always-auth = false
audit = true
auth-type = "legacy"
bin-links = true
browser = null
ca = null
cache = "/Users/vesa/.npm"
cache-lock-retries = 10
cache-lock-stale = 60000
cache-lock-wait = 10000
cache-max = null
cache-min = 10
cafile = undefined
cert = null
cidr = null
color = true
commit-hooks = true
depth = null
description = true
dev = false
dry-run = false
editor = "vi"
engine-strict = false
fetch-retries = 2
fetch-retry-factor = 10
fetch-retry-maxtimeout = 60000
fetch-retry-mintimeout = 10000
force = false
git = "git"
git-tag-version = true
global = false
global-style = false
globalconfig = "/Users/vesa/.nvm/versions/node/v8.9.4/etc/npmrc"
globalignorefile = "/Users/vesa/.nvm/versions/node/v8.9.4/etc/npmignore"
group = 20
ham-it-up = false
heading = "npm"
https-proxy = null
if-present = false
ignore-prepublish = false
ignore-scripts = false
init-author-email = ""
init-author-name = ""
init-author-url = ""
init-license = "ISC"
init-module = "/Users/vesa/.npm-init.js"
init-version = "1.0.0"
json = false
key = null
legacy-bundling = false
link = false
local-address = undefined
loglevel = "notice"
logs-max = 10
; long = false (overridden)
maxsockets = 50
message = "%s"
; metrics-registry = null (overridden)
no-proxy = null
node-options = null
node-version = "8.9.4"
offline = false
onload-script = null
only = null
optional = true
otp = null
package-lock = true
package-lock-only = false
parseable = false
prefer-offline = false
prefer-online = false
prefix = "/Users/vesa/.nvm/versions/node/v8.9.4"
production = false
progress = true
proxy = null
read-only = false
rebuild-bundle = true
registry = "https://registry.npmjs.org/"
rollback = true
save = true
save-bundle = false
save-dev = false
save-exact = false
save-optional = false
save-prefix = "^"
save-prod = false
scope = ""
script-shell = null
scripts-prepend-node-path = "warn-only"
searchexclude = null
searchlimit = 20
searchopts = ""
searchstaleness = 900
send-metrics = false
shell = "/usr/local/bin/zsh"
shrinkwrap = true
sign-git-tag = false
sso-poll-frequency = 500
sso-type = "oauth"
strict-ssl = true
tag = "latest"
tag-version-prefix = "v"
timing = false
tmp = "/var/folders/1d/7yvc5fbs72zdfsqtb0cjbrqh0000gn/T"
umask = 18
unicode = true
unsafe-perm = true
usage = false
user = 501
; user-agent = "npm/{npm-version} node/{node-version} {platform} {arch}" (overridden)
userconfig = "/Users/vesa/.npmrc"
version = false
versions = false
viewer = "man"

@th0r
Copy link
Collaborator Author

th0r commented Sep 11, 2018

Here's the full output of npm config list -l. The registry value has https:// for me, and I suppose that is why my installations always want the https:// part to be in the package lock:

I use Node v10.10.0 and npm v6.4.1 and have https:// protocol set for npm registry as well. Running npm i for me doesn't make any changes to package-lock.json so I don't know what causes the issue.

@valscion
Copy link
Member

Maybe you have some stale cache somewhere that still has set some packages to use http: URLs? Dunno. Doesn't matter too much, though, as it's only for development and doesn't block this PR

@valscion
Copy link
Member

Seems like there's more details about the HTTPS -> HTTP issue here: https://npm.community/t/npm-install-downgrading-resolved-packages-from-https-to-http-registry-in-package-lock-json/1818/4

@valscion
Copy link
Member

Powwow the new checkbox is MAGICAL! Amazing work @th0r 🎉 💯 👍

Seems like this PR is ready, no? I don't see any UI problems anymore, everything is so nice ☺️

Copy link
Member

@valscion valscion left a comment

Choose a reason for hiding this comment

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

💯 👍 🥇 🚀

@th0r
Copy link
Collaborator Author

th0r commented Sep 11, 2018

Powwow the new checkbox is MAGICAL!

Haha, thanks! 😊

Seems like this PR is ready, no?

Yep, seems so! All further features that I have on my mind will be added a little bit later.

@th0r th0r changed the title [WIP] Version 3 Version 3 Sep 11, 2018
@th0r th0r merged commit d7682a6 into master Sep 11, 2018
@th0r th0r deleted the search branch September 11, 2018 13:56
@th0r
Copy link
Collaborator Author

th0r commented Sep 11, 2018

v3 released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants