Skip to content

Conversation

@binh-dam-ibigroup
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup commented Apr 3, 2020

This PR removes icons from lib/components/icons and uses those from the OTP-UI icons package instead. The PR also removes unused narrative profile components.

EDITS: Please see the Remarks section below for edits and testing notes.

Remarks

  1. [Outdated as of 9219719] Full support of custom icons in OTP-ui is pending Add classic icons otp-ui#119, however the refactor is mostly compatible with the currently released OTP-ui library.
  2. [Outdated as of 9219719] In
    const MyLegIcon = TriMetLegIcon // For testing, you can use () => <Ferry />
    , there is a MyModeIcon variable passed to various ModeIcon props, however ModeIcon props will take effect pending a change in SettingsSelectorPanel in Add classic icons otp-ui#119.
  3. [Outdated as of 6c10ea6] As a result, the default icon for "Take Transit" in OTP-RR is changed, and there are subtle visual changes to the itinerary narratives as shown in the screenshots below.
    (Note that the bus and walk icon sizes are pending OTP release per fix(icons): Adjust relative size of bus and walk icons otp-ui#105 to address this comment) Beware, visual changes taking place! (see screenshots below)

image

image

screenshot if using LineItinerary
image

  1. As of 6c10ea6, visual styles for trip form elements (not the map or nav bar) match newplanner.trimet.org, and the old styling is abandoned.

@codecov-io
Copy link

codecov-io commented Apr 3, 2020

Codecov Report

Merging #150 into dev will increase coverage by 0.53%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #150      +/-   ##
==========================================
+ Coverage   10.51%   11.04%   +0.53%     
==========================================
  Files         107       91      -16     
  Lines        3796     3612     -184     
  Branches     1023      968      -55     
==========================================
  Hits          399      399              
+ Misses       2954     2800     -154     
+ Partials      443      413      -30     
Impacted Files Coverage Δ
lib/components/app/default-main-panel.js 0.00% <ø> (ø)
lib/components/app/print-layout.js 0.00% <0.00%> (ø)
...mponents/form/connected-settings-selector-panel.js 0.00% <ø> (ø)
lib/components/form/default-search-form.js 0.00% <0.00%> (ø)
lib/components/form/settings-preview.js 0.00% <ø> (ø)
lib/components/form/styled.js 0.00% <0.00%> (ø)
lib/components/form/tabbed-form-panel.js 0.00% <0.00%> (ø)
lib/components/mobile/main.js 0.00% <0.00%> (ø)
lib/components/mobile/options-screen.js 0.00% <0.00%> (ø)
lib/components/mobile/results-screen.js 0.00% <0.00%> (ø)
... and 11 more

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 20974b3...e0373e8. Read the comment docs.

@evansiroky
Copy link
Contributor

Just noticed that this PR depends on #147. I'm going to unassign myself until that one gets merged into dev.

@evansiroky evansiroky removed their assignment Apr 3, 2020
@binh-dam-ibigroup
Copy link
Collaborator Author

There are some components I deleted an need to bring back... I will reassign you when done.

@evansiroky
Copy link
Contributor

Unassigning myself again until #147 is merged.

@evansiroky evansiroky removed their assignment Apr 6, 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.

See comments regarding adding a new ModeIcon component to otp-ui (in place of the use of TriMetLegIcon in this repo).

@binh-dam-ibigroup
Copy link
Collaborator Author

@landonreed per a suggestion by @evansiroky, I removed the customIcons support but I realized we might still need some folks to occasionally override the std transit or streetcar icons, for instance without having to define a whole new ModeIcon or LegIcon. Let me think through it.

@binh-dam-ibigroup
Copy link
Collaborator Author

@landonreed and @evansiroky, I have updated the opentripplanner/otp-ui packages and updated the examples for custom icon sets accordingly, so you can review.

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, this looks good. I'll condition my approval on merging #164 into this branch.

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.

Great work!

@evansiroky evansiroky removed their assignment May 13, 2020
@codecov-commenter
Copy link

codecov-commenter commented May 21, 2020

Codecov Report

Merging #150 into dev will increase coverage by 0.52%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #150      +/-   ##
==========================================
+ Coverage   10.51%   11.04%   +0.52%     
==========================================
  Files         107       91      -16     
  Lines        3796     3614     -182     
  Branches     1023      968      -55     
==========================================
  Hits          399      399              
+ Misses       2954     2802     -152     
+ Partials      443      413      -30     
Impacted Files Coverage Δ
lib/components/app/default-main-panel.js 0.00% <ø> (ø)
lib/components/app/print-layout.js 0.00% <0.00%> (ø)
...mponents/form/connected-settings-selector-panel.js 0.00% <ø> (ø)
lib/components/form/default-search-form.js 0.00% <0.00%> (ø)
lib/components/form/settings-preview.js 0.00% <ø> (ø)
lib/components/form/styled.js 0.00% <0.00%> (ø)
lib/components/form/tabbed-form-panel.js 0.00% <0.00%> (ø)
lib/components/mobile/main.js 0.00% <0.00%> (ø)
lib/components/mobile/options-screen.js 0.00% <0.00%> (ø)
lib/components/mobile/results-screen.js 0.00% <0.00%> (ø)
... and 12 more

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 20974b3...6c10ea6. Read the comment docs.

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.

LGTM, thanks @binh-dam-ibigroup. I say let's get this merged and start our final review of this.

@landonreed landonreed merged commit 1c053a5 into dev Jun 3, 2020
@landonreed landonreed deleted the otp-ui-icons branch June 3, 2020 14:23
@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.

6 participants