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

Re-order casebook contents via a handle #2031

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

rebeccacremona
Copy link
Contributor

See ENG-336.

You can currently re-order the contents of (draft) casebooks you own by dragging around the entries in the table of contents. The entire entry (that is, the entire white or black rectangle) is the target: you can click/touch anywhere to drag.

That works out fine on desktop, but breaks things on mobile: the touch event to drag takes over any attempts to click the links and buttons inside the entry. So, you cannot take any actions, or navigate the casebook contents.

A person might be able to fix that by handling touch events on all the links and buttons, and doing something like https://github.com/harvard-lil/h2o/compare/develop...rebeccacremona:touch-events-example?expand=1, but I thought it would be simpler to change things, so that you have to drag a handle to reorder.

This PR makes an attempt. I am a) not sure it looks very good, and b) a little anxious that I could have subtly broken something.... but as best I can tell, I have not....

I would definitely appreciate feedback on the appearance, and another person giving the TOC some exercise to see if -they- can find anything I broke.

image

I also tidied the editable view of the table of contents a bit while I was in there, so that things align a little better.
image

@rebeccacremona rebeccacremona requested a review from a team as a code owner October 12, 2023 21:26
@rebeccacremona rebeccacremona requested review from kilbergr and removed request for a team and kilbergr October 12, 2023 21:26
@codecov-commenter
Copy link

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (2cc783f) 76.59% compared to head (3dec823) 76.59%.
Report is 5 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2031   +/-   ##
========================================
  Coverage    76.59%   76.59%           
========================================
  Files           64       64           
  Lines         7066     7066           
========================================
  Hits          5412     5412           
  Misses        1654     1654           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -1,6 +1,6 @@
<template>
<svg height="32" width="32" v-bind:class="{open: !collapsed, collapsed:collapsed}">
<polygon points="6,6 20,16 6,24" />
<svg height="20" width="32" v-bind:class="{open: !collapsed, collapsed:collapsed}">

Choose a reason for hiding this comment

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

Curious about this change — was the height of the svg specifically an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I was trying to get everything aligned, and kind of went on a journey...

image

After the delete button is made to line up, this SVG's box was sufficiently large to affect the overall height of the TOC entry.... such that the black "section" entries, which have the expand/collapse triangle were taller than the white "resource" entries.

And, I noticed that the triangle itself wasn't filling up the box: the box had a lot of empty space in it.... which also made that hard to visually align (evidently leading to those -8px margins etc. that I was able to remove.)

I did make this change BEFORE figuring out align-self:center; and removing some of the other CSS..... so it might be that I didn't need to do this.

height: 36px;
width: 36px;
height: 20px;
width: 20px;
Copy link

@tinykite tinykite Oct 13, 2023

Choose a reason for hiding this comment

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

Curious about this height/width change as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, same as with the SVG: this was to make the delete button align with the little boxes that say "Text" or "Case", and not affect the height of the entry.

Copy link

@tinykite tinykite left a comment

Choose a reason for hiding this comment

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

Let's push this to stage, and I'll test for any unintended bugs there!

@rebeccacremona rebeccacremona merged commit 4bb7ffa into harvard-lil:develop Oct 13, 2023
2 checks passed
@rebeccacremona rebeccacremona deleted the touch-targets branch October 25, 2023 19:47
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.

3 participants