Skip to content

Conversation

@binh-dam-ibigroup
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup commented Mar 23, 2020

This PR replaces TripDetails in lib/components/narrative, TripViewerOverlay in lib/components/map, and PrintableItinerary in lib/components/narrative/printable with their counterparts from OTP-ui.

Visual changes in TripDetails are shown in the screenshot below.
image

Visual changes in PrintableItinerary are shown in the screenshot below.
image

@codecov-io
Copy link

codecov-io commented Mar 23, 2020

Codecov Report

Merging #142 into dev will increase coverage by 0.21%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #142      +/-   ##
==========================================
+ Coverage   10.13%   10.35%   +0.21%     
==========================================
  Files         132      131       -1     
  Lines        5861     5739     -122     
  Branches     1711     1671      -40     
==========================================
  Hits          594      594              
+ Misses       4469     4360     -109     
+ Partials      798      785      -13     
Impacted Files Coverage Δ
lib/components/app/print-layout.js 0.00% <0.00%> (ø)
...ib/components/map/connected-trip-viewer-overlay.js 0.00% <0.00%> (ø)
lib/components/map/default-map.js 0.00% <0.00%> (ø)
lib/components/narrative/connected-trip-details.js 0.00% <0.00%> (ø)
.../components/narrative/default/default-itinerary.js 0.00% <0.00%> (ø)
lib/components/narrative/line-itin/itin-body.js 0.00% <0.00%> (ø)
lib/index.js 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40ae6e7...b72c501. Read the comment docs.

…unterpart from OTP-UI.

Also change PrintLayout to support customIcons from configuration, and remove unused CSS.
@binh-dam-ibigroup binh-dam-ibigroup changed the title Trip Details and Trip Viewer Overlay Trip Details, Trip Viewer Overlay, and Printable Itinerary. Mar 23, 2020
@binh-dam-ibigroup
Copy link
Collaborator Author

I am also adding PrintableItinerary to this PR because that component (actually, PrintLayout) uses TripDetails.

@@ -0,0 +1,25 @@
import { connect } from 'react-redux'
import styled from 'styled-components'
import TripDetailsBase from '@opentripplanner/trip-details'
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if there's a better name for this import. UnstyledTripDetails? I'm fine with what you have, but it does seem like there might be a better name out there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super opinionated about this. When I do this I usually do this the other way around a la:

import TripDetails from  '@opentripplanner/trip-details'
import styled from 'styled-components'

const StyledTripDetails = styled(TripDetails)`
...

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to agree on some kind of convention.

Copy link
Member

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

@binh-dam-ibigroup, does the config need to change at all?

@binh-dam-ibigroup
Copy link
Collaborator Author

There are no changes to the config object as far as I know.

Copy link
Contributor

@evansiroky evansiroky left a comment

Choose a reason for hiding this comment

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

Looks good. Would be nice to agree on the variable naming conventions.

@evansiroky evansiroky removed their assignment Mar 26, 2020
Copy link
Member

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

@binh-dam-ibigroup, a couple of items on the styling:
image

  • The question mark does feel just slightly too big.
  • In the print view, could you scale the ped icon down a little and the bus icon up a little?

@binh-dam-ibigroup
Copy link
Collaborator Author

Unfortunately the size of the question mark icon is hard-coded inside OTP-UI...do we want a ticket for this? Also, the bus and walk icons have these proportions natively in OTP-UI icons, so we would have to either resize the icons in OTP-UI or pollute the code with size adjustments... what do you say?

@binh-dam-ibigroup
Copy link
Collaborator Author

When opentripplanner/otp-ui#104 and opentripplanner/otp-ui#105 get released, you will see the icon size adjustments in OTP-RR. There are no other code work on the OTP-RR repo for integrating trip-details and printable itinerary.

@landonreed landonreed removed their assignment Apr 2, 2020
@binh-dam-ibigroup
Copy link
Collaborator Author

Thanks for the approval. I need to merge the latest changes from dev, then I'll merge this into dev.

@binh-dam-ibigroup binh-dam-ibigroup merged commit 721d403 into dev Apr 2, 2020
@binh-dam-ibigroup binh-dam-ibigroup deleted the otp-ui-trip-details branch April 2, 2020 18:16
@binh-dam-ibigroup binh-dam-ibigroup mentioned this pull request Apr 3, 2020
@landonreed
Copy link
Member

🎉 This PR is included in version 1.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@landonreed
Copy link
Member

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants