Adding map

[ajakubo1]

This is the pull request which adds a possibility of showing map and additional elements (I've prepared and used as an example 4 markers).

I'm missing here:

• fetching data from server (+ I didn't add any fetch polyfill)
• there are a lot of components missing :/
• I didn't have time to add tests for actions/reducers
• Would be neat to mock some data if back-end is not avaliable
• We should add logic which would allow us to decide which marker on the map should be displayed with which pollution level

---

This change is 

[coveralls]

Coverage decreased (-17.9%) to 82.143% when pulling b1c57b5 on adding_map into 060f3b7 on master.

[coveralls]

Coverage decreased (-36.8%) to 63.158% when pulling 79c28d7 on adding_map into 060f3b7 on master.

[lechup]

Generallly , have You seen my markup here: #3 (comment)?

Have a look at whole discussion there.

---

Reviewed 17 of 21 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

webpack.config.js, line 9 at r2 (raw file):

const PATHS = {
    app: path.join(__dirname, 'app'),
    styles: [

This is correct way of doing it? I'm using import './node_modules/whatever/file.css' and it is magicaly added by css-loader.

The same with images only by file loader?

    module: {
        loaders: [
            {
                test: /\.jsx?$/,
                loader: 'babel-loader',
                exclude: /node_modules/,
                query: {
                    presets: ['es2015', 'react', 'stage-0']
                }
            },
            {
                test: /\.less$/,
                loader: 'style!css!postcss-loader!less-loader',
            },
            {
                test: /\.scss/,
                loader: 'style!css!postcss-loader!sass-loader',
            },
            {
                test: /\.css$/,
                loader: 'style!css!postcss-loader',
            },
            {
              test: /\.(png|jpg|gif|svg)$/,
              loader: "file-loader?name=[path][name].[ext]?[hash]"
            }
        ]
    },

Should I read something? :)

---

app/helpers/mapMarkers.js, line 41 at r2 (raw file):

        iconUrl: errorImageUrl,
    }, defaultProperties
));

new line + just thinking - shouldn't all files be either *.jsx or *.js? Or there is some convention I'm not aware of?

---

Comments from Reviewable

[ajakubo1]

@lechup yeah!

There's a webpack tutorial regarding this:

http://survivejs.com/webpack/introduction/

I totally recommend that (thought author updated it recently to using beta version of webpack and some libraries error-out :D).

The idea which this guy has, is to make sure that we are not loading some junk automatically, and so that at a first glance we know what is the structure of project which we load. This also agrees with Zen of Python rule, where 'explicit is better then implicit'. I know we're dealing with js here, but it's a decent rule to follow :).

btw. I did browsed the mockup you refered to, I didn't add any buttons or space for charts, as we're not serving that yet :). Because of that I didn't add any routing library (thought that it might be out-of-scope for that PR).

[coveralls]

Coverage remained the same at 100.0% when pulling bdd38d6 on adding_map into 060f3b7 on master.

[coveralls]

Coverage decreased (-1.5%) to 98.507% when pulling fd53024 on adding_map into 060f3b7 on master.

[coveralls]

Coverage decreased (-5.06%) to 94.937% when pulling 911a2ff on adding_map into 060f3b7 on master.

[coveralls]

Coverage decreased (-11.5%) to 88.506% when pulling 7b3c507 on adding_map into 060f3b7 on master.

[coveralls]

Coverage remained the same at 100.0% when pulling f520865 on adding_map into 060f3b7 on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 3c4d344 on adding_map into 060f3b7 on master.

[ajakubo1]

@lechup - I think you can take a look now, and if it's looking OK, we can finally merge with master.

So basically, as of now, if you update the station API to handle get filtering via NE, SW points, then we can have the stations on the map with pollution levels.

This last metering value per station should be serialized as value (let me know if you serialize it under something else, as I'd have to refactor some code in order for this to work with frontend).

After the merge, I'll take a look at Victory lib.

[ajakubo1]

Oh, actually - that's not 100% right, as I input some dummy-data after fetch is done, no matter the response :).

[lechup]

Generally ok, we need some base to work on ;)

---

Reviewed 3 of 8 files at r5, 1 of 2 files at r6, 3 of 4 files at r7, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

package.json, line 36 at r8 (raw file):

    "enzyme": "^2.6.0",
    "extract-text-webpack-plugin": "^1.0.1",
    "fetch-mock": "^5.8.1",

JS does not have generic mock library like python? Shouldn't all mocks be grouped in packages.json with all test related libraries? Just asking I do not have the foggies idea ;)

---

app/request.js, line 4 at r8 (raw file):

import queryString from 'query-string';

export const FAKE_DOMAIN = 'http://fakedomain.com/';

FAKE_DOMAIN is used in tests? Do we have API_URL somewhere? Hm... Maybe it should have deault of '' but we should be able to override it while setting environment variable?

---

app/actions/index.js, line 29 at r8 (raw file):

        dispatch(invalidateLocationData());
        dispatch(fetchDataForLocation(northEast, southWest));
        return getAPI(API_GET_STATION_DATA, {

I'm not sure how it will be parsed but it should be something like here: https://github.com/djangonauts/django-rest-framework-gis#inbboxfilter

---

app/actions/index.js, line 40 at r8 (raw file):

                            position: [52.229, 21.032],
                            description: "This marker should be blue",
                            value: null

I plan to provide all data from Metering model in 'last_metering' key,

---

app/components/PollutionMap.jsx, line 17 at r8 (raw file):

        this.state = {
            mapCenter: [52.229, 21.011],

From my experience it is better to keep bouding box instead of map center in state. But if we do not plan to provide urls that should show same results on different resolutions for now it's ok.

---

Comments from Reviewable

[ajakubo1]

Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

package.json, line 36 at r8 (raw file):

Previously, lechup (Leszek Piątek) wrote…

JS does not have generic mock library like python? Shouldn't all mocks be grouped in packages.json with all test related libraries? Just asking I do not have the foggies idea ;)

Nope, it doesn't :). Hence the leftpad crisis :D.

Actually - I've updated packages.json via npm install --save-dev <package_name> - so that might cause some chaos in library listings we have :). I can rearrange them, but I don't think that's a thing :D.

---

webpack.config.js, line 9 at r2 (raw file):

Previously, lechup (Leszek Piątek) wrote…

This is correct way of doing it? I'm using import './node_modules/whatever/file.css' and it is magicaly added by css-loader.

The same with images only by file loader?

    module: {
        loaders: [
            {
                test: /\.jsx?$/,
                loader: 'babel-loader',
                exclude: /node_modules/,
                query: {
                    presets: ['es2015', 'react', 'stage-0']
                }
            },
            {
                test: /\.less$/,
                loader: 'style!css!postcss-loader!less-loader',
            },
            {
                test: /\.scss/,
                loader: 'style!css!postcss-loader!sass-loader',
            },
            {
                test: /\.css$/,
                loader: 'style!css!postcss-loader',
            },
            {
              test: /\.(png|jpg|gif|svg)$/,
              loader: "file-loader?name=[path][name].[ext]?[hash]"
            }
        ]
    },

Should I read something? :)

Haven't responded here - I was basing the configuration on this tutorial: http://survivejs.com/webpack/introduction/

---

app/request.js, line 4 at r8 (raw file):

Previously, lechup (Leszek Piątek) wrote…

FAKE_DOMAIN is used in tests? Do we have API_URL somewhere? Hm... Maybe it should have deault of '' but we should be able to override it while setting environment variable?

Yep, its only for tests. I plan to list all API urls in urls.js.

The way I do it know - is that when I'm trying to determine the domain for tests, I add the FAKE_DOMAIN thing to url resolve (createUrl function). For production and live testing, this is not added anywhere.

Sure - I like your idea, we can just pre-fill some variable here which we use all the time.

---

app/actions/index.js, line 29 at r8 (raw file):

Previously, lechup (Leszek Piątek) wrote…

I'm not sure how it will be parsed but it should be something like here: https://github.com/djangonauts/django-rest-framework-gis#inbboxfilter

Example url in this configuration: /station/northEast=LatLng%2852.24378%2C%2020.98251%29&southWest=LatLng%2852.16456%2C%2020.86338%29

---

app/actions/index.js, line 40 at r8 (raw file):

Previously, lechup (Leszek Piątek) wrote…

I plan to provide all data from Metering model in 'last_metering' key,

good to know :). I'll swap the name then.

---

app/components/PollutionMap.jsx, line 17 at r8 (raw file):

Previously, lechup (Leszek Piątek) wrote…

From my experience it is better to keep bouding box instead of map center in state. But if we do not plan to provide urls that should show same results on different resolutions for now it's ok.

I need to have a map center here, as it's an argument I'm passing during render. And re-render :). I was thinking of adding a bounding box here, but came to a conclusion that I need it only when I fetch for station data, so - why bother saving it anywhere?

---

app/helpers/mapMarkers.js, line 41 at r2 (raw file):

Previously, lechup (Leszek Piątek) wrote…

new line + just thinking - shouldn't all files be either *.jsx or *.js? Or there is some convention I'm not aware of?

nope. *.jsx are the components which are using html tags inside. No need to name everything either js or jsx.

New lines are my nemesis :3. I should really enable eslint support at some time in the future.

---

Comments from Reviewable

[lechup]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

package.json, line 36 at r8 (raw file):

Previously, ajakubo1 (Adam Jakubowski) wrote…

Nope, it doesn't :). Hence the leftpad crisis :D.

Actually - I've updated packages.json via npm install --save-dev <package_name> - so that might cause some chaos in library listings we have :). I can rearrange them, but I don't think that's a thing :D.

Ok, leave it like that.

---

app/request.js, line 4 at r8 (raw file):

Previously, ajakubo1 (Adam Jakubowski) wrote…

Yep, its only for tests. I plan to list all API urls in urls.js.

The way I do it know - is that when I'm trying to determine the domain for tests, I add the FAKE_DOMAIN thing to url resolve (createUrl function). For production and live testing, this is not added anywhere.

Sure - I like your idea, we can just pre-fill some variable here which we use all the time.

Of course I mean API_URL not FAKE_DOMAIN, we could inject it in docker-compose.yml if we need to.

---

app/actions/index.js, line 37 at r8 (raw file):

                        // TODO it should have data from repsonse
                        {
                            type: METRIC_TYPES.PM10,

Hm.. what is type for? We have such field in API, but it's HW version of station not METRIC_TYPE...

---

app/actions/index.js, line 40 at r8 (raw file):

Previously, ajakubo1 (Adam Jakubowski) wrote…

good to know :). I'll swap the name then.

It will be rather last_metering.pm10 or last_metering.pm25.

---

Comments from Reviewable

[ajakubo1]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

package.json, line 36 at r8 (raw file):

Previously, lechup (Leszek Piątek) wrote…

Ok, leave it like that. :lgtm_strong:

Done.

---

app/actions/index.js, line 37 at r8 (raw file):

Previously, lechup (Leszek Piątek) wrote…

Hm.. what is type for? We have such field in API, but it's HW version of station not METRIC_TYPE...

My workaround on... well... Type of metric sent :). I'll have to change it somehow based on your comment last_metering.pm10, last_metering.pm25. I use that type later on to figure out what color of marker should I use on the map (I didn't remove that decision code, as - based on our discussion - I'm not 100% sure how do we represent live-data on the map).

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 100.0% when pulling caf4815 on adding_map into 060f3b7 on master.

[lechup]

let's merge it to master!

---

Reviewed 9 of 9 files at r9.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable