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

dashboard: base setup #505

Merged
merged 2 commits into from Mar 2, 2018

Conversation

Projects
None yet
5 participants
@thelostone-mc
Copy link
Member

commented Feb 27, 2018

Description

Reason for this PR
Rather than working on explorer page and submitting a PR with tons of changes.
I'd wanna break it into batches (small deliverables ) => easier to review and merge

This PR has two commits

  • added support for Muli font family
  • reindent /explorer page and added missing closing tag

This would be the first of the series.
Me and @eswarasai will start on the explorer page soon. Figured I'd get some cleaning up done beforehand to make our work easier

Checklist
Refers/Fixes

cc: @owocki

@owocki

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

i wish there was an easy way to say via github diffs that this is just formatting changes without having to do it line by line :/

this lgtm; @mbeacom ?

@thelostone-mc thelostone-mc changed the title dashboard: reindented and added missing closing tag dashboard: base setup Feb 27, 2018

@thelostone-mc

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2018

@owocki I also added another commit here!
Just added Muli fonts to our code base

@eswarasai

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2018

@thelostone-mc -- I've added .editorconfig to ensure that the spacing issues won't occur in the future and also to follow standard indentation throughout the project. Can you please try to configure your IDE to use the local .editorconfig and fix the spacing issues?

cc: @owocki @mbeacom

@thelostone-mc

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2018

@eswarasai thoughts on shifting to 2 spaces = 1 tab for html pages ?
#487 (comment)

@coderberry

@eswarasai

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2018

@thelostone-mc -- Sure. If that's what ideal and preferable, then we can definitely remove html from 4-space indentation.

@owocki @mbeacom -- Any objections here?

@owocki

This comment has been minimized.

Copy link
Member

commented Feb 28, 2018

defer to @mbeacom on this one.. ill do whatever the group does

@mbeacom

mbeacom approved these changes Mar 1, 2018

Copy link
Contributor

left a comment

lgtm - I'm fine with the indentation change. I think it would be beneficial for us to document the styling guidelines, though.

@mkosowsk

This comment has been minimized.

Copy link

commented Mar 1, 2018

@mbeacom

Is it in the roadmap to use something like Prettier to codify code formatting?

@mbeacom

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2018

@mkosowsk Yes! We're currently in the deliberation stage of determining an approach to automatic styling and enforcement of style. I'd love to remove the guess work as much as possible for community contributors and reduce the friction (as well as manual interaction/intervention) for contributions.

I'm open to discuss how we tackle this moving forward.

@mkosowsk

This comment has been minimized.

Copy link

commented Mar 2, 2018

@mbeacom Using Prettier seems relatively straightforward:

  1. Install dependencies
  2. Configure prettier.rc with JSON of agreed upon styles (bracketSpacing, tabWidth, etc.)
  3. I'm using TypeScript so here I use a TSLint but Gitcoin would use an ESLint. Seems that there are some issues with using Prettier and a lint but looks like they're straightforward to resolve:

https://stackoverflow.com/questions/44690308/whats-the-difference-between-prettier-eslint-eslint-plugin-prettier-and-eslint

  1. Configure .lintstagedrc
  2. Add appropriate scripts to package.json that people can run before submitting PR's
  3. Contributors now can write and commit pretty code by just running a script after
    doing their work 😍

Might be interesting to open up to the community what type of standards they think are good to enforce 👍🏻

Think this is a very important topic to tackle so that contributors can focus their energy on more crucial things, like whether or not to use emoji in the codebase. I personally give that idea two 👍 way up!

@mbeacom mbeacom merged commit 2773140 into gitcoinco:master Mar 2, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.