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 social media icons #329

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open

Add social media icons #329

wants to merge 37 commits into from

Conversation

erwok
Copy link

@erwok erwok commented Aug 29, 2022

No description provided.

@erwok erwok self-assigned this Aug 29, 2022
@erwok
Copy link
Author

erwok commented Aug 29, 2022

I'm confused on why backend is failing. I haven't touched it.

@erwok erwok requested a review from kyle1373 October 15, 2022 19:21
@erwok
Copy link
Author

erwok commented Oct 15, 2022

After ten years 🙏🙏🙏

flexDirection: 'column-reverse',
height: '100%',
borderCollapse: 'collapse',
paddingLeft: '2px',
Copy link
Member

Choose a reason for hiding this comment

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

I would make paddingLeft with the value 0px. This would center the buttons instead of having them slightly more aligned to the right than normal

Copy link
Author

Choose a reason for hiding this comment

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

Added!

@@ -33,6 +33,33 @@ const styles = theme => ({
h5: {
marginBottom: theme.spacing(2),
},
container: {
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't have the name container as it seems too general. Maybe a better name would be socialContainer.

Copy link
Author

Choose a reason for hiding this comment

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

Added!

Copy link
Member

@kyle1373 kyle1373 left a comment

Choose a reason for hiding this comment

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

Great implementation! Please check the two comments I made, but beyond that, it looks good to me.

Copy link
Author

@erwok erwok left a comment

Choose a reason for hiding this comment

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

I incorporated the suggestions

kyle1373
kyle1373 previously approved these changes Nov 3, 2022
Adneths
Adneths previously approved these changes Nov 13, 2022
Copy link
Collaborator

@Adneths Adneths 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 overall

}


export const SocialButtons = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since it's not actually exporting a Button element, it may be clearer to rename it to Info or Link instead of button, same for other instances and file name.

laurenkiyomi
laurenkiyomi previously approved these changes Nov 15, 2022
Copy link
Contributor

@laurenkiyomi laurenkiyomi 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 to me other than renaming the "Button" elements.

@kyle1373 kyle1373 dismissed stale reviews from laurenkiyomi and Adneths via eb96ae5 November 23, 2022 02:44
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.

5 participants