freeCodeCamp / 2016-new-coder-survey

196 stars 61 forks source link

Add map preloader #60

Closed SamAI-Software closed 7 years ago

SamAI-Software commented 7 years ago

Website preview is here


@krisgesling, it's a quick fix, so may be later you can make preloader better and more flexible to changes. The idea is to set height to all DOM elements before the user is anchored to a topic from url ID (.../#Podcast). You don't set height for the map's legend in the code, so I basically set it to 70px, which might be a problem if somebody will change the legend's content or CSS for it and will forget to adjust 70px in map.js.

krisgesling commented 7 years ago

Nice work, thanks!

Just in terms of the wording change, I don't think it's correct grammar to say the 'new coders are ethnic minority'. It makes it sound like 'new coders' is a type of 'ethnic minority'. I guess the simplest wording I can think of is 'new coders who are from an ethnic minority in their country'.

I may change the jQuery to d3 or vanilla js and call the preloader from within maps.js just to remove dependencies for future projects. Doesn't matter so much in this context given the rest of the page uses jQuery already.

SamAI-Software commented 7 years ago

I don't think it's correct grammar to say the 'Proportion of new coders who are ethnic minority in their country.'

Otherwise it won't fit on the small screens :) It's the easiest way. Of course, there are other solutions through CSS, but they are not so easy, because we have 4 maps in the same container, which should keep the same height. You know your code better, so feel free to fix that, but make sure that there will be no overlapping on 320px screens.

krisgesling commented 7 years ago

"Proportion of new coders who are from an ethnic minority in their country." seems to work down to 300px screen width.

On Sun, Jul 17, 2016 at 10:42 AM Sam Aiken notifications@github.com wrote:

I don't think it's correct grammar to say the 'Proportion of new coders who are ethnic minority in their country.'

Otherwise it won't fit on the small screens :) It's the easiest way. Of course, there are other solutions through CSS, but they are not so easy, because we have 4 maps in the same container, which should keep the same height. You know your code better, so feel free to fix that, but make sure that there will be no overlapping on 320px screens.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/FreeCodeCamp/2016-new-coder-survey/pull/60#issuecomment-233159620, or mute the thread https://github.com/notifications/unsubscribe-auth/AB9o2tq8_1Edb-JWgxsdRFNqDKxz939eks5qWYGMgaJpZM4JN-yH .

krisgesling commented 7 years ago

There's also two console.logs left in barcharts.js, not sure if you want to remove those before it goes live too.

SamAI-Software commented 7 years ago

@krisgesling done :+1:

QuincyLarson commented 7 years ago

Excellent work!