The 3to5 from fac-11

'x' as element in forEach

It might seem a big picky (probs cause it is a bit) 😳 but on line 45 of dom.js you use x as the argument to the callback to your forEach.

Generally I think it's nicer to put a descriptive name to the element being iterated over by an array method than just x or something. Think about what x in this instance actually is and maybe replace it with something more descriptive.

This will make your code more readable, cause code is for humans!

Responsive, mobile-first design

Design: aim for a responsive, mobile-first design

Must be functional on all browsers, screen resolutions.

Must be able to deal with latency issues; mobile devices without wifi.

OpenWeatherMap to Build Current weather app

Your app features some dynamic content

Update without refreshing the page using AJAX?

Implement postcode validation

Should be able to return an error if the postcode entered is invalid:

function valid_postcode(postcode) {
postcode = postcode.replace(/\s/g, "");
var regex = /[A-Z]{1,2}[0-9]{1,2} ?[0-9][A-Z]{2}/i;
return regex.test(postcode);
}

Non responsive

Begins to break at width 750px

Greater depth of walking instructions

At the moment the instructions are really on useful for public transport. Walking and cycling need more detailed instructions to actually be useful.

Small code for input and output

Break your JavaScript down into small functions with a clear input and output; this will make it easy to write tests

requires refresh to reenter data

For example, if mistake is made, or data is unacceptable.

Is the site accessible by all?

Accessibility: same as always, we'll be running your code through accessibility checkers

getweather not defined in dom.js

dom.js:43 Uncaught ReferenceError: getWeather is not defined
at HTMLButtonElement. (dom.js:43)

Accessibility - Journey Details - Too much separation between duration, mode icon and instructions

If you can recall last week's talk on accessibility, it is better to have the elements a lot closer to each other otherwise it can be hard to read the information flow.

On the desktop version of the site, there is a lot of separation across the screen between these elements.

Perhaps you could implement something that is similar to TFL's website, see screenshot below:

Put your stretch goals in as issues

You've got a really nice list of stretch goals in your readme, you could raise each of these as issues, perhaps making a label called "stretch-goal" and then they would be retained in history in a nice github way.

No feedback on bad data

Bad data, non- London postcodes, part postcodes eg WC1A and some other postcodes, eg E98 1AT will stall the page, as data cannot be processed, but cannot be reentered. An error message would fix this.

A well-considered architecture for your app

Think back to the workshops from the beginning of this week. Try to modularise your code, or break it down into separate files. Document any key decisions about how you structure your code in your readme!

Use of github

you've related quite a few PRs and commits to issues, but still not all of them, please make sure to relate an issue in every commit and PR.

If you are writing a commit message and think "there's not an issue I can relate this to" then make that issue. All work you are doing should be done in relation to an issue.

Issues are the source of truth for your project, they should be where you find out who worked on what, which commits related to it, what was done, and when.

If in doubt, raise more issues than you think you need than less.

Accessibility - add alt text to mode icons

In the summary and journey details there is no alt text for the icons. This could be added to help with accessibility

Hosting?

Is this hosted anywhere? (github pages, surge, other) If so plz put a link in your repo!

A clearly defined user journey, documented in your readme.

Flush out requirements in README

Summary should be hidden initially

Before postcode has been entered, don't want summary to be visible

Spelling error in weather__suggestions class

Don't forgot to bring your vitamin D supplements

An easy fix - Replace forgot with forget

Tests: you must write tests!

We'll need to experiment with the API's first before we focus on the tests.

Should we TDD?

Create Icons for the mode of transportation.

We need easily read icons to represent the mode of transport summary at the top of the page.

Change colour based on line?

Icons and arrows to represent the journey?

API Key privatisation

If using a private API key, you won't be able to deploy to GitHub pages this week (if you're not using an API key, go for it!)

Return a message if fac postcode is entered

if the fac postcode is entered there should be a message on screen saying 'you're already here!'

add sub heading and detail of what the page is for

Add subheading : "probably the best weather travel app ever conceived"
detail: "Forget the days of being late to FAC, just enter your postcode and we'll get you to FAC in no time"

Colours could use some work

GOD the colour scheme is atrocious! Please change for the good of all!

Security and Privacy

Take appropriate measures when it comes to concealing private information (i.e. your API key!)

combine the two 'onclick' methods in dom.js

L8 and L76

impure function (which could be pure)

your getTravel method both constructs the url and calls the function that makes the api call.

If you made getTravel return the URL, and removed the call to logic.request on line 24, and then passed the result of getTravel(postcode) into logic.request then your getTravel (probably more aptly named makeTravelUrl or similar at that point) function would be pure and you could write some nice tests for it.

Suggestions for what to bring

e.g its raining, bring your brolly etc

Form has body properties

When we merged some property for the body went to the form. This stops the responsiveness.

Your app queries at least two APIs using the XMLHttpRequest method

Our app should use two APIs at least.
From initial brainstorms we are using the following as a minimum:

TFL to map journey
Weather App - Open weather map API?

As extras can map to google map/ overlay.

Travel warnings -Delays, maintenance, etc...

We could have a red warning icon/message if the planned route is expected to have delays

Dom should be passed weather info

e.g. temperature, description

Build Journey using TFL API

Build the main journey using the TFL API

Update readme to include proper api key info

Just actualising the IRL conversation we just had as an issue 😉
Make sure you have correct instructions for set up in your readme, as the curent ones for the api keys don't work.

Though I really appreciate the effort to hide your api keys, on a front end only app it is not possible (on a hosted version someone would still be able to look in "config.js" in their console) so don't worry too much about it, you'll be a ble to hide API keys when you start building your own servers.

istanbul cover tape test/*.test.js

No coverage information was collected, exit without writing coverage information

CSS - Add padding and specific elements for the journey details

CSS

Add padding as too close to the sides for the body
Add specific id tags to the CSS so that the details section's css doesnt not affect or get affected by the rest of the page

Accessability information for the journey

We could add information about stairs, long walks etc at various stops on the journey.

fac-11 / 3to5 Goto Github PK

3to5's People

Watchers

3to5's Issues

Recommend Projects

Recommend Topics

Recommend Org