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

Line Diagram | Collapsible branches #345

Merged
merged 3 commits into from
Jan 16, 2020
Merged

Conversation

thecristen
Copy link
Collaborator

@thecristen thecristen commented Jan 13, 2020

Summary of changes

Asana Ticket: Line Diagram | Collapsible branches

  • Adds styles for collapsed and expanded branches. It's a delightful mix of JavaScript logic, SVG attributes, rather complex CSS rules...
  • Breaks up the larger React component into a few smaller ones
  • Reuses the existing <ExpandableBlock /> component to handle collapsing/expanding sections of the line diagram
  • Bugfix - removes the "View schedule" link from the terminus stops
  • IE fix - removes the usage of "unset" in our CSS
  • Apparently I'm the first to use a lodash function here... so I moved the package from devDependencies.

Screenshots!

IE11 preview

Video: https://app.crossbrowsertesting.com/public/i31732a965037ca8/livetests/35727589/videos/z9d8a4cb074e2a020941?test_hash=e0aed05e

Firefox preview

Screen Shot 2020-01-13 at 14 55 38

Safari preview

Screen Shot 2020-01-13 at 14 56 50

Chrome preview

localhost_4001_schedules_Green_line_direction_id=0 (1)

@ingridpierre
Copy link

Really small presentational changes. Can we:

  • increase the border radius, so that the expand/collapse control has more rounded shoulders.
  • vertically center the three dots more inside the expansion control
  • make the lines thinner (it's taking a lot of room in mobile especially), instead of 10px and 16px wide, let's try 8 and 12?

Current:
image

Desired (roughly):
image

apps/site/assets/css/_shuttles-page.scss Show resolved Hide resolved
@@ -17,6 +17,7 @@
"jquery": "> 3.0.0",
"leaflet": "^1.4.0",
"leaflet-rotatedmarker": "^0.2.0",
"lodash": "^4.17.11",
Copy link
Contributor

Choose a reason for hiding this comment

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

This change doesn't seem to have propagated to package-lock.json (I think something should have flipped to dev: false?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not exactly sure how package-lock is supposed to reflect this change, but I've run the update and committed what it gave me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had thought since we moved a package from devDependencies to dependencies that this would be reflected in the lockfile by that dependency flipping from "dev": true to "dev": false. But since nothing changed (aside from the usual optional fights), I'm thinking we already had a package in our production dependencies that was pulling in Lodash as a dependency of its own, hence it was already marked as "dev": false.

tl;dr: we don't have to commit this change to the package-lock 😅

@thecristen
Copy link
Collaborator Author

@ingridpierre @digitalcora Made some updates that should address the comments here.

@codecov
Copy link

codecov bot commented Jan 13, 2020

Codecov Report

Merging #345 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #345   +/-   ##
=====================================
  Coverage      94%    94%           
=====================================
  Files         356    356           
  Lines        5918   5918           
=====================================
  Hits         5563   5563           
  Misses        355    355

@ingridpierre
Copy link

Looks so good, one teeny adjustment -- can we reduce the padding on .m-schedule-diagram__stop from 1rem all the way around to: 1rem .5rem? Decreasing the horizontal padding wins just a tiny bit more line length, which I'd like to take where I can get it :)

@thecristen
Copy link
Collaborator Author

thecristen commented Jan 14, 2020

@ingridpierre done!
@digitalcora I've gone ahead and renamed first/last to origin/destination and also was able to deprecate the isEnd prop!

I've also updated the logic to expand short branches by default. So now Line Diagram | Don't collapse short branches is covered.

Copy link
Contributor

@digitalcora digitalcora left a comment

Choose a reason for hiding this comment

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

Looking good @thecristen, I'm glad we have your CSS and React wizardry on this task 🙇‍♀

@digitalcora digitalcora removed their assignment Jan 15, 2020
@thecristen thecristen force-pushed the cbj-linediagram-collapsing branch from 993825b to 8518b1d Compare January 15, 2020 19:17
@digitalcora
Copy link
Contributor

@thecristen Looks like some things you extracted to separate commits are also still present in the commit message for the "main" commit.

- Splits the larger LineDiagram component into smaller ones
- Reuses the existing <ExpandableBlock /> component to handle collapsing/expanding sections of the line diagram
- Adds styles for collapsed and expanded branches
- Bugfix - removes the "View schedule" link from the terminus stops
@thecristen thecristen force-pushed the cbj-linediagram-collapsing branch from 8518b1d to f6f8d52 Compare January 15, 2020 20:22
@thecristen thecristen merged commit 115dc64 into master Jan 16, 2020
@thecristen thecristen deleted the cbj-linediagram-collapsing branch January 23, 2020 00:33
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.

4 participants