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

/app/pages/common/Modals/PreferencesModal/Badges/index.js 🔨 Refactor 🧠 Overmind Hacktoberfest #2760

Merged
merged 5 commits into from
Oct 22, 2019
Merged

/app/pages/common/Modals/PreferencesModal/Badges/index.js 🔨 Refactor 🧠 Overmind Hacktoberfest #2760

merged 5 commits into from
Oct 22, 2019

Conversation

Sowmiya07
Copy link
Contributor

@Sowmiya07 Sowmiya07 commented Oct 13, 2019

Refactored the badges index.js file to use overmind and added named export instead of default export

What kind of change does this PR introduce?

This is part of the requested refactor for hacktoberfest mentioned in #2760
@Saeris @christianalfoni

What is the current behavior?

Previously it was utilizing inject and hooksObserver from app/componentConnectors.

What is the new behavior?

Prefers useOvermind over inject and other small stylistic choices mentioned in #2760

What steps did you take to test this? This is required before we can merge.

yarn:lint: I was not able to do this as I have some errors doing yarn
yarn:test: I was not able to do this as I have some errors doing yarn

Checklist

  • Documentation
  • Testing
  • Ready to be merged
  • Added myself to contributors table

refactored the badges index.js file to use overmind and added named export instead of default export
@vercel vercel bot temporarily deployed to staging October 13, 2019 10:31 Inactive
@vercel
Copy link

vercel bot commented Oct 13, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/codesandbox/codesandbox-client/9jik58ugm
🌍 Preview: https://codesandbox-cl-git-fork-sowmiya07-refactor-badges-to-use-098bf2.codesandbox1.now.sh

@Sowmiya07 Sowmiya07 changed the title refactored badges to use overmind /app/pages/common/Modals/PreferencesModal/Badges/index.js 🔨 Refactor 🧠 Overmind Oct 13, 2019
@Sowmiya07 Sowmiya07 changed the title /app/pages/common/Modals/PreferencesModal/Badges/index.js 🔨 Refactor 🧠 Overmind /app/pages/common/Modals/PreferencesModal/Badges/index.js 🔨 Refactor 🧠 Overmind Hacktoberfest Oct 13, 2019
@ganes1410
Copy link
Contributor

@Sowmiya07 I think this is failing because of lint error. Maybe try running yarn lint and try to push again

@@ -1,12 +1,20 @@
import React from 'react';
import { inject, observer } from 'app/componentConnectors';
import React, {FunctionComponent} from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import React, {FunctionComponent} from 'react';
import React from 'react';

For types we don't need to use named imports, we can just pull it off React


import Margin from '@codesandbox/common/lib/components/spacing/Margin';
import Badge from '@codesandbox/common/lib/utils/badges/Badge';
import { Title } from '../elements';

function BadgesComponent({ store, signals }) {
const badgesCount = store.user.badges.length;
export const Badges: FunctionComponent = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const Badges: FunctionComponent = () => {
export const Badges: React.FC = () => {

export const Badges: FunctionComponent = () => {
const {
state: {
user: {badges},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
user: {badges},
user: { badges },

eslint --fix should have caught this, not sure why it didn't. Adding spaces for consistency

user: {badges},
},
actions: {
preferences: {setBadgeVisibility},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
preferences: {setBadgeVisibility},
preferences: { setBadgeVisibility },

Copy link
Contributor

@Saeris Saeris left a comment

Choose a reason for hiding this comment

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

Looks great! Made a few change requests suggesting stylistic changes for consistency with the rest of the code base. Could you add them all to a batch and commit them? You can do that from the files tab. I'd do it myself but I can't commit to your fork. Thanks for the submission!

@SaraVieira
Copy link
Contributor

Thank you for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧠 Overmind Indicates that this is related to the app's State Management 🔨 Refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants