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

Create a basic waffle chart #36

Merged
merged 6 commits into from
Oct 28, 2019
Merged

Create a basic waffle chart #36

merged 6 commits into from
Oct 28, 2019

Conversation

puripant
Copy link

This partially fulfills #21 without dropdown selection and tooltip.

@rapee
Copy link
Contributor

rapee commented Oct 25, 2019

@puripant It seems you're working on an old master;profile.yaml now renamed people.yaml. and some attribute names have changed to this one:

{
  id
  fields {
    slug
  }
  title
  name
  party_position
  is_cabinet
  is_senator
  is_mp
}

@rapee
Copy link
Contributor

rapee commented Oct 25, 2019

@puripant
Copy link
Author

@rapee Not sure what you want me to do. Should I scrape the whole thing and start over by pulling from the newly updated data or should you merge this one and then I fix it from there?

@puripant
Copy link
Author

Also, the data dictionary could have answer my questions in #21 and it should be posted there as well.

@p16i
Copy link
Contributor

p16i commented Oct 25, 2019

Hi,

I have a comment. I think we should include style with the code, instead of having a separate file.

@puripant
Copy link
Author

puripant commented Oct 25, 2019

Hi,

I have a comment. I think we should include style with the code, instead of having a separate file.

@heytitle Is there any particular reason for that? By the way, this waffle chart may be used in other parts. We may have to create a component based on this.

@p16i
Copy link
Contributor

p16i commented Oct 25, 2019

imho, it's easier to maintain. For example, we used this approach in elect-live.

@puripant
Copy link
Author

I am slightly partial to the traditional style mainly because of CSS reuse through CSS classes. I guess I can define an object to do the same thing in JS anyway. However, my old-fashioned mind still cares about the separation of content and presentation ... It's up to @rapee to set up the project's coding style then.

@rapee
Copy link
Contributor

rapee commented Oct 26, 2019

Current setup supports both styles.

For traditionalists, you can use one of the two styles:

  1. Global CSS, by import ./styles/my_page.css
  2. CSS Module, by
import classes from './styles/my_button.module.css'
<div className={classes.container}>{children}</div>

For modernists, use controversial CSS-in-JS.

<div css={{ marginTop: 10, marginBottom: '1rem' }}>{children}</div>

In my opinion, if you're developing apps with highly component-based structured, CSS-in-JS is surprisingly intuitive and easy to use. Even for traditionalist like me.

@puripant
Copy link
Author

@rapee Got it about CSS. Will change later. Can you reply to my earlier question about what I should do with this pull request?

@rapee
Copy link
Contributor

rapee commented Oct 28, 2019

@puripant You can add new commits on top of this branch and do git push. This PR is receiving any new commits automatically.

@puripant
Copy link
Author

puripant commented Oct 28, 2019

@rapee I did that but I don't know what else you want me to change

@rapee
Copy link
Contributor

rapee commented Oct 28, 2019

Merge basic waffle @puripant

@rapee rapee closed this Oct 28, 2019
@rapee rapee reopened this Oct 28, 2019
@rapee rapee merged commit 085d493 into electinth:master Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants