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

Improve tree view #87

Closed
wants to merge 33 commits into from
Closed

Conversation

lapo-luchini
Copy link
Owner

I'm creating this PR myself for ease of tracking olibu's branch as quoted in #81.

I have concept draft for a new interactive tree view based on the latest esm branch in my repository. https://github.com/olibu/asn1js/tree/treeview
What do you think about it.

@lapo-luchini
Copy link
Owner Author

I really like the "real tree" look, I'm not very fond of the ⊕⊖ icons, seems a bit too cumbersone (I mean too "noticeable" in regards of the actual text), but that's a minor issue (I think it can be improved easily with smaller size or with less contrast).

Something funny happened with distances between elements (my 2.0.1 vs your treeview):
image
image

@lapo-luchini
Copy link
Owner Author

I'm not very fond of the ⊕⊖ icons, seems a bit too cumbersome

Might also be the reason the new view occupies more vertical space.

Was it intended change (i.e. for visual clarity?) or a unintended side-effect?

@olibu
Copy link
Contributor

olibu commented Apr 20, 2024

Thank you for your feedback!
I'm not really happy with the icons, too. They have larger and black and white. I'll reduce them a little bit more to reduce the distraction.
I have not changed any vertical spacing explicitly. I can hardly see the differences. I expect that this is an side effect of the icons and the <li> elements.
Shall I add more space between the lines again?

@olibu
Copy link
Contributor

olibu commented Apr 20, 2024

I've update the feature branch with less distracting tree icons.
As part of the tree stylesheet I had removed too many spacing styles. After recovering, the UI now looks much more like the original one.

I've deployed that version at https://olibu.github.io/asn1js/

Waiting for your feedback.

@lapo-luchini
Copy link
Owner Author

Much better!
image
About vertical space… it does use a bit more space, but watching it again maybe it was a bit "too packed" before, rather than "too spacey" in the new one. 🤔

@lapo-luchini
Copy link
Owner Author

There are a few "weird spots" where the tree lines have double width, can you check that?
PS: I can't see any new commit with these fixes on the feature branch… did you forget to push?

@olibu
Copy link
Contributor

olibu commented Apr 21, 2024

I do not see any of this "weird spots". Maybe it's dependent on the resolution? I'm using Chrome and Safari on a Mac.
image

All changes are now available at https://github.com/olibu/asn1js/tree/feature/treeview
There have been some push issues as you already found out ;)

@olibu
Copy link
Contributor

olibu commented Apr 21, 2024

I can reproduce the "weired spots" when zomming the page. Some zoom levels show the lines in different sizes. I will try to find a fix for this.

@olibu olibu force-pushed the feature/treeview branch from ed7ddd9 to 0d84f33 Compare April 22, 2024 09:46
@lapo-luchini
Copy link
Owner Author

@olibu I'm going to fix this squashed in a single commit, it would be too difficult to re-build all changes in single commits, I hope you don't mind. (I usually prefer 1:1 commits myself…)

index.js Outdated
@@ -231,3 +237,19 @@ selectTag.onchange = function (ev) {
let tag = ev.target.selectedOptions[0].value;
window.location.href = 'https://rawcdn.githack.com/lapo-luchini/asn1js/' + tag + '/index.html';
};

// zoom fix to have straight lines in treeview
Copy link
Owner Author

Choose a reason for hiding this comment

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

Is it possible to do this using CSS rules instead of custom JS?
Because I fear that direct style rules such as this would be stopped by CSP on official website.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a version without JS. Looks fine on my screen.

lapo-luchini pushed a commit that referenced this pull request May 12, 2024
From 64546e4
Date: Sun, 28 Jan 2024 18:17:45 +0100
Subject: [PATCH 01/25] hex copy on click

From 151289b
Date: Sun, 28 Jan 2024 22:08:09 +0100
Subject: [PATCH 02/25] Context menu and copyAsString added

From 5efe7f7
Date: Mon, 29 Jan 2024 08:26:40 +0100
Subject: [PATCH 03/25] Context menu initially hidden

From 36dd239
Date: Mon, 29 Jan 2024 13:28:54 +0100
Subject: [PATCH 04/25] Fix: Copy the string to clipboard

From 01b1525
Date: Mon, 29 Jan 2024 13:31:51 +0100
Subject: [PATCH 05/25] New: Copy as pretty

From 4978b90
Date: Mon, 29 Jan 2024 13:33:13 +0100
Subject: [PATCH 06/25] New: Align context menu to left

From c7cc3ba
Date: Mon, 29 Jan 2024 18:48:31 +0100
Subject: [PATCH 07/25] Negative list for set and sequence

From c69ed8d
Date: Mon, 29 Jan 2024 18:52:28 +0100
Subject: [PATCH 08/25] Fix: Show context menu at correct position

From 0b9b3f3
Date: Mon, 29 Jan 2024 19:22:45 +0100
Subject: [PATCH 09/25] Simple marker with toggle method

From 8eb7457
Date: Wed, 31 Jan 2024 19:50:58 +0100
Subject: [PATCH 10/25] Indention fixed

From 80e7a8b
Date: Sun, 31 Mar 2024 19:35:37 +0200
Subject: [PATCH 11/25] Menu added to tree view

From 6dcb057
Date: Sun, 31 Mar 2024 22:09:03 +0200
Subject: [PATCH 12/25] Pretty text from innerText

From 88d7afd
Date: Wed, 10 Apr 2024 21:01:10 +0200
Subject: [PATCH 13/25] new: tree view

From 59d44dc
Date: Fri, 19 Apr 2024 15:42:51 +0200
Subject: [PATCH 14/25] Only highlight the current selected node

From 1dc1323
Date: Fri, 19 Apr 2024 15:48:44 +0200
Subject: [PATCH 15/25] smaller padding as of tree layout

From 60383f9
Date: Fri, 19 Apr 2024 16:17:50 +0200
Subject: [PATCH 16/25] Smaller borders and icon

From a6cf850
Date: Fri, 19 Apr 2024 16:31:23 +0200
Subject: [PATCH 17/25] do not show the menu in case of clicking the icon

From 174df2c
Date: Fri, 19 Apr 2024 16:38:45 +0200
Subject: [PATCH 18/25] Installation of node to execute lint correctly

From 48fbe0b
Date: Fri, 19 Apr 2024 16:38:54 +0200
Subject: [PATCH 19/25] Singlequotes

From fbd2b26
Date: Sat, 20 Apr 2024 13:33:30 +0200
Subject: [PATCH 20/25] Only show context menu on highlighted element

From ea3c28f
Date: Sat, 20 Apr 2024 14:09:29 +0200
Subject: [PATCH 21/25] Less offensive tree icons

From 1f9913c
Date: Sat, 20 Apr 2024 14:28:33 +0200
Subject: [PATCH 22/25] Original layout recovered

From 0d84f33
Date: Sat, 20 Apr 2024 15:02:26 +0200
Subject: [PATCH 23/25] Layout issue fixed

From ba21d40
Date: Mon, 22 Apr 2024 19:46:31 +0200
Subject: [PATCH 24/25] Fix: Zoom issues of the border line in treeview

From d303e65
Date: Tue, 23 Apr 2024 09:35:51 +0200
Subject: [PATCH 25/25] Indentation fixed
@lapo-luchini
Copy link
Owner Author

lapo-luchini commented May 12, 2024

BTW: I squashed your branch in 76b5048 (and partially merged to current trunk).

@lapo-luchini
Copy link
Owner Author

I merged this PR in branch github-87, which can be tested online on githack.

@olibu
Copy link
Contributor

olibu commented May 12, 2024

The branch looks fine to me. The githack link however has some CSS issues. Maybe the cache has to be reloaded.

@lapo-luchini
Copy link
Owner Author

Maybe the cache has to be reloaded.

Probably; that happened to me as well, but after a reload with F12+disable cache it looked fine.

@lapo-luchini
Copy link
Owner Author

Merged in 88cb856.

@olibu olibu deleted the feature/treeview branch July 30, 2024 13:32
nobecutan pushed a commit to nobecutan/asn1decode that referenced this pull request Sep 3, 2024
1383174 Merge pull request #1 from lapo-luchini/trunk
88cb856 propagate from branch 'it.lapo.asn1js.github-87' (head 1c8c4956cd564008f7eed9da4b4c5339c41877ab)             to branch 'it.lapo.asn1js' (head 9f75cfa39a7b636c5f645b78d7c1ead1ca738473)
440b6c9 Recover dark theme changes from GitHub ea3c28fdcc467f2f8c20dd97747e27f3add2605b
1a42055 Use className/classList.
7e187c8 Drop empty lines.
28d859d `btnHideTree` is no longer needed.
f4df0c8 Zoom fix via css media query instead of JS
85c143f Drop overridden dependency. (else `npm` is broken; this way `pnpm` has a warning but still works)
e1b6d4d `npx svgo *.svg` (except "src" favicon, that could need to be edited)
dc0eeba Fix dark collapse icon.
0d4d50a Use a code style closer to trunk branch.
4c1706f propagate from branch 'it.lapo.asn1js' (head 2836ee93ead827f06d611376950b3de3012c96e1)             to branch 'it.lapo.asn1js.github-87' (head e3919c3c3e940015d73441c4379c5337ed084457)
f2a2ddd Forbid trailing spaces.
f03d403 propagate from branch 'it.lapo.asn1js.github-90' (head 82116fdcf1c3b63bcde16e03867bdcad1eb2d051)             to branch 'it.lapo.asn1js' (head cc6d74d0a3dc372a270fac7069edd9c55bd4cac9)
9a0a01e Small theme refactoring.
3fd828a Import module natively, not in the HTML.
c921739 Fix peer dependencies.
06b6f3c theme.js added to lint
37883bf theme-color set for iOS devices
041d7b6 Theme support for single file
844d85b Theme moved into own js file as of performance issues
c1554d2 Common css instead of css replacement
6c01961 Update tags.
a490f08 Version 2.0.4.
5930b5b Add CLI binary.
ea72cc2 Improve single-file mode.
8e1cfed Add links.
aba062a Update tags.
06d6fcc Add instruction for local usage.
51fb9c5 propagate from branch 'it.lapo.asn1js.esm' (head 38124bce4601268d8065c1abd3114977330f32c9)             to branch 'it.lapo.asn1js' (head 5ce4e6aff2b2fefa6f9db72f33d8632fb6959607)
c4b53e5 Version 2.0.3.
a71445e Add defs to npm package. Linting.
5caf633 merge of '7612a59a7e8328fb3cdd183616a8455c779d2aa2'      and 'a773502d82c5ca9f65a9d2026a029ad908bcd34c'
8e1cc32 Update require-ESM docs.
7a4d218 Improve ESM usage examples.
af73d5d Fix nodejs example usage.
260a7a5 Drop dead code.
b7e8538 Force favicon as dataURI in "local" file.
f20564c eslint.
9679f24 Add `pnpm` support. (why doesn't it support `npm` override format too?)
e6a247e Update ignore files.
e3dd726 Advertise now single-file HTML.
5cda4f7 Since ESM is not useable on `file:` protocol, use Vite to generate a single-file `index-local.html`.
f4fc630 Only associate buttons' `onClick` when they exist. (to allow removing them)
8dc103c Only install when needed by `lint`.
ee8643f Install listed `eslint` to avoid auto-installing a `9.x` version which has breaking changes.
76b5048 Squashed rebase of changes from lapo-luchini/asn1js#87
f2f3e2a Export decode functions. (needed by VSCode extension)
826f20c Add encrypted file example.
7cec5a1 Update tags.
6ae6072 Version 2.0.2.
6f4b911 Improve optional type matching.
89a776d Fix `NULL` support further.
57b9ce4 `NULL` is a built-in type.
7a871a8 Upgrade actions from deprecated Node 16 to 20.
5e7deb8 Install listed `eslint` to avoid auto-installing `9.1.0` which breaks with our configuration format.
fd1b6d0 Add new test to check for defs regressions.
8400fa7 Allow specifying expected type.
3f5e5fa Add support for data URI input.
20c1581 propagate from branch 'it.lapo.asn1js' (head 2af3ae813887be59bf3e2e123a12e77592921fa8)             to branch 'it.lapo.asn1js.esm' (head 95bda12bb582ab39529f02626bab93554a4dd214)
6fa69ff Add X.509 SubjectPublicKeyInfo to default types.
3bc6092 Add missing types to RFC parser.
6052316 Improve hover highlight.
b6c0059 Use `classList` instead of manipulating `className`.
bbd561f Fix sha256sums.
ce7a05a Lint new file too.
513085b propagate from branch 'it.lapo.asn1js' (head 2c2471b462a45fe5bbd48c9d9347f2df5ab039d9)             to branch 'it.lapo.asn1js.esm' (head 8fc43cda40cb66f028d050df54b1e506736d094a)
ad3d920 `eslint --fix`
cce28c5 Print defs also in pretty tring mode.
dc57878 Drop old forgotten "collapse" icon.
e41f826 Add context menu also on the tree on the left.
270561b Rename context menu values. Add B64. Put optional one last to keep button positions.
5908ead Avoid inline javascript. (for future usage with CSP header)
4a03d93 Refactor context menu to use existing methods. (and avoid re-parsing parsed data)
fe04d97 Use blocks instead of `<br>` in context menu.
0eb4fcf Improve hex dump capabilities of ASN.1 nodes.
3291c10 File forgot in previous (refactor) commit.
0c2dc0c Refactor contextual menu in a different module.
c2ebbf4 propagate from branch 'it.lapo.asn1js.esm' (head 1f81e568f0723f8ce942b435f81d205e157789ce)             to branch 'it.lapo.asn1js.github-82' (head 7107a48abf688173dcdf7bd716d415c8edd85f46)
e3fca65 Update credits.
be8d97a propagate from branch 'it.lapo.asn1js' (head dd18afd1c4344811526fc928f3975fe414584018)             to branch 'it.lapo.asn1js.github-82' (head 27ed876d23a39e4ec2bfd2f6233f38610720add2)
1fcc683 Force examples permissions.
aa1f1c3 propagate from branch 'it.lapo.asn1js' (head 741a5e9cfd4ef4e45f1e5b505e02c4142e82dd8d)             to branch 'it.lapo.asn1js.esm' (head 4d3ff39de9922b95cbd11c2534c9188ff1db72f0)
2654e9c Add very basic PKCS#1 support and RSA key examples.
5027c72 propagate from branch 'it.lapo.asn1js' (head a9524436000d6561058ba0ae8e2a58a6c49837fd)             to branch 'it.lapo.asn1js.esm' (head a6b77044e2cf0f91cb663d9d1fbfbca7f66c2d21)
8f048ac When trying to parse encapsulated values, check that the content can be parsed as well.
405c036 Drop dead code.
7a51b8a Fix tests.
26d3a72 Throw exceptions, not strings.
9ff23e1 Use HTTPS links.
c894e6a Index only needs to import a single module.
8663924 Version 2.0.1.
70426d2 Update tags.
65c222d Update browser support warnings.
b7f47f6 Improve README.
f220f66 propagate from branch 'it.lapo.asn1js' (head 263fa0cb3936ec99c2be2c51a2265cadfbded687)             to branch 'it.lapo.asn1js.esm' (head c360c489896f2e9ec2c280e6d55b03a0581252c2)
690364a Improve type matching.
ae4458e Update example usage.
1b3078e Version 2.0.0 in order to signal this is a breaking change.
ae7ee7b Unbreak tests.
72c71f1 Update copyright.
c1f4dff Convert to ES6 Modules (ESM).
429f752 Indention fixed
1d7f90b Fix: Show context menu at correct position
e6dbc08 Negative list for set and sequence
dcf1b13 New: Align context menu to left
af5e9e7 New: Copy as pretty
d5465fc Fix: Copy the string to clipboard
e7d2bfd Context menu initially hidden
17dd53a Context menu and copyAsString added
c7cf816 hex copy on click
7c2a63b Update `eslint` rules a bit.

git-subtree-dir: asn1decode/asn1js
git-subtree-split: 138317433032d91debc2eea51536beadb757835d
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.

2 participants