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

Adds apple-touch-icon to /bus-arrival/ #29

Merged
merged 7 commits into from Jan 15, 2019
Merged

Adds apple-touch-icon to /bus-arrival/ #29

merged 7 commits into from Jan 15, 2019

Conversation

@jglim
Copy link
Contributor

@jglim jglim commented Nov 29, 2018

Default busrouter.sg apple-touch-icon added to /bus-arrival/, inserted
inline as it may be overwritten below:

On /bus-arrival/, when a bus is pinned, the default apple-touch-icon is
overridden with a new canvas-generated icon for that specific bus


Here's a screenshot of the generated apple-touch-icon ("Clementi")

4348bfd

I've also uploaded a screen recording of the result

  • 00:00 to 00:04 : default behavior with the new apple-touch-icon
  • 00:04 to 00:13 : changes to apple-touch-icon after pinning
  • 00:13 to 00:17 : launching from the bookmark (no address bar yay!)
Default busrouter.sg apple-touch-icon added to /bus-arrival/, done
inline as it may be overwritten below:

On /bus-arrival/, when a bus is pinned, the default apple-touch-icon is
overridden with a new canvas-generated icon for that specific bus
Copy link
Owner

@cheeaun cheeaun left a comment

Few notes:

  • Since we're already setting a custom icon, why not set a custom app title as well? By doing this <meta name="apple-mobile-web-app-title" content="XXX">
  • Would also be cool if also can generate an icon for a bus stop too, instead of just a bus service number.
  • However, what happens when someone "stars" more than one bus service code? 🤔

bus-arrival/index.html Outdated Show resolved Hide resolved
…web-app-title

- "apple-touch-icon-precomposed" is now "apple-touch-icon"
- apple-touch-icon links to a png instead of inlining base64
- Added apple-mobile-web-app-title that contains the current bus stop's
name
@jglim
Copy link
Contributor Author

@jglim jglim commented Nov 29, 2018

Thanks for the quick reply!

  • Since we're already setting a custom icon, why not set a custom app title as well? By doing this <meta name="apple-mobile-web-app-title" content="XXX">

Sure! I used to save the stop names based on intent (e.g. 196 "Home » Clementi"), though displaying the stop name might be semantically better, so I implemented it as such.

  • Would also be cool if also can generate an icon for a bus stop too, instead of just a bus service number.

Static image (can!) or dynamic content generated from text (trickier, still can..)? Will first listen to and understand your thoughts before implementing.

  • However, what happens when someone "stars" more than one bus service code?

The last "starred" service code will be the one that's used as the icon 😆 Will be happy to change that, though I am not sure how to approach it yet.

fd73924

  • "apple-touch-icon-precomposed" is now "apple-touch-icon"
  • apple-touch-icon links to a png instead of inlining base64 (still works)
  • Added apple-mobile-web-app-title that contains the current bus stop's
    name

screen recording

@cheeaun
Copy link
Owner

@cheeaun cheeaun commented Jan 12, 2019

Hey @jglim sorry for the late reply.

After some thinking, here are my thoughts:

  • The icon shows bus stop code instead of bus service number.
  • App title will show bus stop name instead. Since it's editable, users can still rename it to whatever they want (Maybe bus service numbers for your use-case).

There can be a lot of permutations for this but let's keep it simple for now 😁

JinGen Lim added 3 commits Jan 15, 2019
On rendering, apple-touch-icon is replaced with the bus stop code

busNumber -> iconText (semantics only)

Icon color changed to match existing busrouter's style - from green (bus numbers) to red (bus stops).

Icon text size is reduced to half of previous, since bus stop codes are universally 5 characters wide
@jglim
Copy link
Contributor Author

@jglim jglim commented Jan 15, 2019

Hi @cheeaun - no problem 😃
The changes requested are available in 807d5e9

screen recording, again

assets/arrival.js Outdated Show resolved Hide resolved
bus-arrival/index.html Outdated Show resolved Hide resolved
utils/setIcon.js Outdated Show resolved Hide resolved
utils/setIcon.js Outdated Show resolved Hide resolved
…-arrival/

Replaced querySelectorAll to querySelector for queries returning one element in arrival.js and setIcon.js (total of 2 instances)

Shifted non-critical meta tags below link tags in bus-arrival/index.html

Changed strings to match existing style (single quotes)
@jglim
Copy link
Contributor Author

@jglim jglim commented Jan 15, 2019

Thanks for reviewing 807d5e9. I've implemented the requested changes in 333f590. There are no visual changes, so I didn't upload a recording.

bus-arrival/index.html Outdated Show resolved Hide resolved
Moved link tag for apple-touch-icon below preloads
@jglim
Copy link
Contributor Author

@jglim jglim commented Jan 15, 2019

Done! Sorry I didn't catch that one 😅

@cheeaun cheeaun merged commit da0b492 into cheeaun:master Jan 15, 2019
5 checks passed
5 checks passed
@netlify[bot]
Pages changed 1 new file uploaded
Details
@netlify[bot]
Header rules 3 header rules processed
Details
@netlify[bot]
Mixed content No mixed content detected
Details
@netlify[bot]
Redirect rules 3 redirect rules processed
Details
@netlify[bot]
deploy/netlify Deploy preview ready!
Details
@jglim
Copy link
Contributor Author

@jglim jglim commented Jan 15, 2019

@cheeaun Thanks for taking time to help this PR through!

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

Successfully merging this pull request may close these issues.

None yet

2 participants