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

Add atoms charts aa1 to implement 1 #27

Merged
merged 16 commits into from
Jun 6, 2020
Merged

Conversation

pinterid
Copy link
Member

@pinterid pinterid commented Jun 6, 2020

Checklist:

  • Have you added an explanation of what your changes do and why you'd like them to be included?
  • Have you updated or added documentation for the change?
  • Have you tested your changes with successful results?

Type of Changes:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Documentation (non-breaking change which adds documentation)
  • Breaking change (fix or feature that would cause existing functionality to change)

What is the current behavior? (link to any open issues here)

What is the new behavior (if this is a feature change)?

Other information:

  • Ref: 🐍

The chart for the 2D calendar has been added.
It is not displayed yet.
The chart for the 3D calendar has been added.
It is not displayed yet.
The chart for the contribution radar
has been added. It is not displayed yet.
The chart for the latest activity
has been added. It is not displayed yet.
Descriptions for the usage of all atoms
have been added.
There are only newlines to separate internal
and external imports in the import section now.
@pinterid pinterid added the enhancement New feature or request label Jun 6, 2020
@pinterid pinterid added this to the SNEK Version 0.1.0 milestone Jun 6, 2020
@pinterid pinterid requested a review from schettn June 6, 2020 12:47
@pinterid pinterid self-assigned this Jun 6, 2020
Copy link
Member

@schettn schettn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some suggestions.

src/components/atoms/charts/Calendar2D/index.jsx Outdated Show resolved Hide resolved
src/components/atoms/charts/Calendar2D/index.jsx Outdated Show resolved Hide resolved
src/components/atoms/charts/Calendar2D/index.jsx Outdated Show resolved Hide resolved
src/components/atoms/charts/Calendar3D/index.jsx Outdated Show resolved Hide resolved
src/components/atoms/charts/Calendar3D/index.jsx Outdated Show resolved Hide resolved
Most of the suggestions have been applied.
All other comments will be resolved imidiately.

Co-authored-by: Pinterics David <55298934+pinterid@users.noreply.github.com>
@snek-at snek-at deleted a comment from pinterid Jun 6, 2020
@snek-at snek-at deleted a comment from pinterid Jun 6, 2020
Copy link
Member

@schettn schettn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some errors produced by the previous commit

src/components/atoms/charts/Calendar3D/index.jsx Outdated Show resolved Hide resolved
src/components/atoms/charts/Calendar2D/index.jsx Outdated Show resolved Hide resolved
@snek-at snek-at deleted a comment from pinterid Jun 6, 2020
schettn and others added 3 commits June 6, 2020 15:21
All other suggestions are implemented now.
The quality of the code has been improved.
The code quality was improved due to a request by Codacy.
@schettn schettn self-requested a review June 6, 2020 14:04
Copy link
Member

@schettn schettn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Copy link
Member Author

@pinterid pinterid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make some changes.

src/components/atoms/charts/Calendar2D/index.jsx Outdated Show resolved Hide resolved
src/components/atoms/charts/ContribRadar/index.jsx Outdated Show resolved Hide resolved
src/components/atoms/charts/ContribRadar/index.jsx Outdated Show resolved Hide resolved
src/components/atoms/charts/LatestActivity/index.jsx Outdated Show resolved Hide resolved
src/components/atoms/index.js Outdated Show resolved Hide resolved
schettn and others added 2 commits June 6, 2020 16:42
All suggestions are implemented now.

Co-authored-by: Pinterics David <55298934+pinterid@users.noreply.github.com>
Now it is a valid heading.

Co-authored-by: Pinterics David <55298934+pinterid@users.noreply.github.com>
The naming of a constant variable
has been refined.
Copy link
Member Author

@pinterid pinterid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good after a change.

The code quality has been improved due to a request by @pinterid.
@kleberbaum kleberbaum self-requested a review June 6, 2020 14:56
Copy link
Member Author

@pinterid pinterid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Copy link
Member

@kleberbaum kleberbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be ok to merge after a few changes.

The code quality has been improved
due to a request of @kleberbaum.
@schettn schettn requested a review from kleberbaum June 6, 2020 15:50
Comment out all console.log
Copy link
Member

@kleberbaum kleberbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be ok to merge.

@schettn schettn merged commit 1661d95 into implement-1 Jun 6, 2020
@pinterid pinterid mentioned this pull request Jun 7, 2020
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants