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

[M1466] Offline Toggle Components (with test) + Refresh project (UI only) #21

Merged
merged 7 commits into from
Feb 19, 2021

Conversation

tienhoah
Copy link
Contributor

No description provided.

@tienhoah tienhoah requested a review from mmnoo February 17, 2021 01:10
Copy link
Contributor

@mmnoo mmnoo left a comment

Choose a reason for hiding this comment

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

Im going to do a little research on this react-switch for the next hour. Ill get back to you asap

import Switch from 'react-switch'

/**
* Describe your component
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you rather remove the component description or add a description? In this case, does the component name do a good enough job?

boxShadow="0px 1px 5px rgba(0, 0, 0, 0.6)"
activeBoxShadow="0px 0px 1px 10px rgba(0, 0, 0, 0.2)"
uncheckedIcon={false}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel strongly against styling via props in return statements/markup. Ok for prototypes, not ideal for maintainable code.

Im going to do a quick foray into how hard it would be to roll out own toggle component, to see if thats a cost effective option here. (have never done).

Copy link
Contributor

Choose a reason for hiding this comment

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

Also going to check if we can style react-switch via styled components as a option

Copy link
Contributor

Choose a reason for hiding this comment

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

K, this is what I came up with #22

...utils,
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It goes against a lot of what we are taught as devs, but its best to avoid the DRY principle in tests. I'd remove the setup function here.

https://vanslaars.io/blog/dont-dry-your-tests/

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise, looks good :)

@mmnoo
Copy link
Contributor

mmnoo commented Feb 17, 2021

@tienhoah tienhoah closed this Feb 19, 2021
@tienhoah
Copy link
Contributor Author

Used alternative one, close this

@mmnoo mmnoo reopened this Feb 19, 2021
@mmnoo
Copy link
Contributor

mmnoo commented Feb 19, 2021

Just reopening to because alternate had this branch and the merge target instead of develop. (a bit confusing set up, sorry)

@mmnoo mmnoo merged commit 64fa8bb into develop Feb 19, 2021
@mmnoo mmnoo deleted the M1466 branch December 9, 2021 17:38
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.

2 participants