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

collapsible pin pane #178

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

peterzhangsnail
Copy link

@peterzhangsnail peterzhangsnail commented Mar 17, 2020

Add an icon to the pin pane to make it collapsable.
Here's how it may look on the ecma doc:
GIF

Closes tc39/ecma262#1898

@ljharb ljharb added the feature label Mar 18, 2020
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks!

js/menu.js Outdated Show resolved Hide resolved
js/menu.js Outdated Show resolved Hide resolved
js/menu.js Outdated Show resolved Hide resolved
js/menu.js Outdated Show resolved Hide resolved
js/menu.js Outdated Show resolved Hide resolved
js/menu.js Outdated Show resolved Hide resolved
js/menu.js Outdated Show resolved Hide resolved
js/menu.js Outdated Show resolved Hide resolved
css/elements.css Outdated Show resolved Hide resolved
@ljharb ljharb changed the title collapsable pin pane collapsible pin pane Mar 18, 2020
@peterzhangsnail
Copy link
Author

@ljharb Here's the new PR: use the method already existed to achieve it, just like your suggestion, looking for more tests and ideas.

@ljharb ljharb reopened this Mar 21, 2020
@peterzhangsnail
Copy link
Author

@ljharb Done!

@ljharb ljharb force-pushed the pinCollapsable branch 2 times, most recently from 0766845 to 4da66d9 Compare March 21, 2020 20:53
@ljharb ljharb self-assigned this Mar 21, 2020
@ljharb ljharb requested review from bakkot, michaelficarra and syg March 21, 2020 20:54
@bakkot
Copy link
Contributor

bakkot commented Apr 20, 2020

I feel like the orientation of the arrows in the screenshot is the opposite of what I expect. Specifically, I expect "down" to mean "expanded" in this context. Compare Github's <details>:

example

@ljharb
Copy link
Member

ljharb commented Apr 20, 2020

Ah, good call - i didn't notice before, but you're right. The triangles should match the way the TOC sections already work - which is as you describe.

@peterzhangsnail, can you fix?

@peterzhangsnail
Copy link
Author

@ljharb I run my build and the orientation of the button seems correct. Here's my process below:
(OS: win7 / build tool: git-bash)
build

@peterzhangsnail
Copy link
Author

@ljharb It's also easy to 'fix' anyway. Just a little exchange of strings in elements.css.

@ljharb
Copy link
Member

ljharb commented May 4, 2020

@peterzhangsnail when "closed", the arrow should be pointing left/inwards (at the title), not right/outwards.

@rkirsling
Copy link
Member

rkirsling commented May 4, 2020

@ljharb It's true that an right-facing arrow on the right often means "go to detailed view" but I'm not sure I can think of an example of an left-facing arrow on the right being used for a closed drawer?

I think when it's on the right, the norm may be upward/downward—see the "pending reviewers" box on this very page, as well as the Preferences pages for Chrome and FF.

@ljharb
Copy link
Member

ljharb commented May 4, 2020

@rkirsling i'd expect "up/down", or "inwards/downwards". Chrome's preference arrows, at least, aren't expanders, they're navigation "go to"s.

this arrow points inwards/downwards, for example (here's some example content)

@rkirsling
Copy link
Member

Yep, I agree with your rationale, I just don't recall seeing an inward-facing arrow on the right.

Chrome and FF Preferences have drawers, you just have to look around for them:
image

image

@ljharb
Copy link
Member

ljharb commented May 4, 2020

Ah, true. Perhaps the "pins" arrow should be on the left, then?

@peterzhangsnail
Copy link
Author

@ljharb That doesn't match with the toc header below it if we put the arrow on the left.

@peterzhangsnail
Copy link
Author

@ljharb Here's the upward/downward version of pin arrow:
(Better change the transition time to 0s, a 180-degree rotate with non-zero transition time seems weird.)
2020-05-04_160409
2020-05-04_160422

@ljharb
Copy link
Member

ljharb commented May 4, 2020

@peterzhangsnail those are backwards too; the downwards arrow in an up/down is the one that shows the contents; upwards hides them.

@peterzhangsnail
Copy link
Author

@ljharb Click a downward/upward arrow then it shows/hides the content. You must click it first.

@peterzhangsnail
Copy link
Author

@ljharb Here's a gif to show the process:
GIF

@ljharb
Copy link
Member

ljharb commented May 4, 2020

Yes, that's backwards. When the content is hidden, the arrow should reflect that by pointing upwards. Clicking it should show the content and thus change it to point downwards, indicating that the content is visible (and that clicking it will make it invisible).

@peterzhangsnail
Copy link
Author

@ljharb Hope this is what we want:
GIF

@peterzhangsnail
Copy link
Author

@ljharb I have run my build routine with the new commit, everything works just fine.

@ljharb
Copy link
Member

ljharb commented May 6, 2020

The things we have to decide between:

  1. disclosure triangle is on the left, closed === points inwards
  • Pros: consistency with TOC triangles
  • Cons: P in "Pins" does not align with "T" in "Table of Contents"
  1. disclosure triangle is on the right, closed === points inwards
  • Pros: "closed" look is consistent with TOC triangles
  • Cons: right-alignment is inconsistent with TOC triangle alignment
  1. disclosure triangle is on the right, closed === points upwards
  • Pros: ?
  • Cons: right-alignment is inconsistent with TOC triangle alignment; "closed" state is inconsistent with TOC triangles

My preference is 1, 2, 3.

@rkirsling
Copy link
Member

I don't have a strong opinion here, but I'll note that the upward/downward approach used by GitHub/Chrome/Firefox has downward as the closed state (I was momentarily confused by this myself, hence all the edits to my first comment above).

@ljharb
Copy link
Member

ljharb commented May 6, 2020

@rkirsling in

content
?

@rkirsling
Copy link
Member

@ljharb What? That's not upward/downward.

@ljharb
Copy link
Member

ljharb commented May 7, 2020

@rkirsling right; so what are you referring to that's up/down in those browsers? <details> is the only thing i'm aware of in html.

@rkirsling
Copy link
Member

@ljharb I just re-referenced what I described in my first two comments: the screenshots as well as the "pending reviewers" box on this page.

@ljharb
Copy link
Member

ljharb commented May 7, 2020

@rkirsling ah, thanks, i see what you mean. In those cases they're not triangles, they're chevrons, and i think the difference, while subtle, actually matters quite a bit.

@rkirsling
Copy link
Member

Hmm, that's fair. Maybe that means it's confusing to have a filled triangle flip instead of rotate then? 🤔 In any event, I have no qualms with your order of preference.

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

Successfully merging this pull request may close these issues.

Making the PIN panel collapsable would get better experience.
4 participants