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

Modificación de Hexdex #627

Merged
merged 3 commits into from
Aug 28, 2018
Merged

Conversation

ogsilvaa
Copy link
Contributor

@ogsilvaa ogsilvaa commented Aug 27, 2018

Se modificó la página para que lea el json de chapters y coloque los logos de cada ciudad ordenado por
región.
'-----------------------------------------------------------------------------------------------------------------
The page was modified to read the chapters json and place the logos of each city ordered by region.

image

#603

@fforres
Copy link
Member

fforres commented Aug 27, 2018

@ogsilvaa This is so coool!


Buenísima maestro! :)

@fforres fforres self-assigned this Aug 27, 2018
@fforres
Copy link
Member

fforres commented Aug 28, 2018

@ogsilvaa

Checking the PR a bit more.
The main idea to serve an already processed HTML is to provide an incremental approach for the site. (In a progressively enhanced way)

/index.html shows all information right at the start (And enhances with a map in case JS is enabled 🤘)

Master branch - index.html Without JS
Master branch - index.html With JS

In comparison,
On this PR, with JS disabled, site does not show any logos. 😕
This branch - hexdex.html Without JS
This branch - hexdex.html With JS

Not sure if we should go down that path :/

@fforres
Copy link
Member

fforres commented Aug 28, 2018

On more personal notes about the design choices

  • I think the idea of having some visual description for the hexagons :)
    Maybe adding them and linking it to the image as an aria-describedby could be cool? 😄

  • The borders for some images have different height :/

screen shot 2018-08-27 at 9 04 12 pm

@fforres
Copy link
Member

fforres commented Aug 28, 2018

@ogsilvaa
I created an issue so we can discuss over there, so we can keep the PR for code-related things :) #628

@fforres fforres mentioned this pull request Aug 28, 2018
@ogsilvaa ogsilvaa merged commit 152f8a7 into nodeschool:source Aug 28, 2018
@fforres
Copy link
Member

fforres commented Aug 28, 2018

😕

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.

3 participants