Graph selection info #53
Conversation
| @@ -15,6 +15,8 @@ import { CollapsibleSection } from './collapsible-section.js'; | |||
| import { NumberBox } from './number-box.js'; | |||
| import { TextButton } from './buttons.js'; | |||
| import { downloadSvg } from './util.js'; | |||
| import { Tooltip } from './tooltip.js'; | |||
There was a problem hiding this comment.
This file is getting bigger and bigger. I wonder whether it makes sense to split it into multiple ones.
There was a problem hiding this comment.
I think we can create an issue to split some of these files up, because this is not the only one getting very big. It will take a bit of time, so it probably makes sense to do this in a different PR.
src/util.js
Outdated
|
|
||
| // remove unnecessary preceding 'www.' and etc from url | ||
| export function shortenUrl(url) { | ||
| const remove = ['http://', 'https://', 'www.']; |
There was a problem hiding this comment.
One corner case: what if www is not the leading characters, such as this URL: mywww.com?
There was a problem hiding this comment.
I am not sure whether it is a good idea to remove the leading www. Technically speaking, www is part of the domain name, so http(s)://www.foobar.com is not necessarily identical to http(s)://foobar.com.
There was a problem hiding this comment.
This function is meant for display purposes only, not for navigation. It's just supposed to make the url look nicer when displayed, and decrease the horizontal space it takes up. When the user clicks on a url in a dynamic field, the full value including any http:// or www is displayed.
There was a problem hiding this comment.
I see. But still, you don't want to display mywww.com as my.com.
There was a problem hiding this comment.
I could do more advanced filtering, like removing everything before the host, or possibly even using the javascript URL functionality to get the relevant bits of information, but I didn't think it was worthwhile to do since our database seems to be fairly clean. It was meant as just a very quick and basic UI optimization. Note that that function is only used in one place and on one specific field of a specific query. @dhimmel thoughts on necessity of this.
There was a problem hiding this comment.
URL seems an overkill. What I meant is something like this:
const targets = ['^http://', '^https://', '^www.'];
for (const str of targets) {
regex = new RegExp(str);
url = url.replace(regex, '');
}
(I renamed remove because using verb to name an array seems a little weird.)
src/path-graph.js
Outdated
| }))(SelectedNodeInfo); | ||
|
|
||
| // selected edge info component | ||
| class SelectedEdgeInfo extends Component { |
There was a problem hiding this comment.
I see some code duplication in this class and SelectedNodeInfo class. How hard is it going to be to remove the duplication (such as defining a abstract class that will be inherited by SelectedNodeInfo and SelectedEdgeInfo)?
There was a problem hiding this comment.
Incorporated.
src/util.js
Outdated
|
|
||
| // remove unnecessary preceding 'www.' and etc from url | ||
| export function shortenUrl(url) { | ||
| const regexes = ['^http://', '^https://', '//www.']; |
|
@dongbohu Was able to fix most of the npm warnings. Turns out but per this comment it's out of our control it seems like. |
|
@vincerubinetti: Thanks. That is good enough. Please feel free to file another PR (or put in #54). |
Adds node/edge selection and its relevant info
Closes #31