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

Review skeleton #73

Merged
merged 7 commits into from May 22, 2022
Merged

Review skeleton #73

merged 7 commits into from May 22, 2022

Conversation

zman811
Copy link
Contributor

@zman811 zman811 commented May 21, 2022

In this pull request, I added the basic file structure as well as some basic placing of elements on the page in the right spots. If someone could look over the file structure as well as the basic styling that would be great

@zman811 zman811 linked an issue May 21, 2022 that may be closed by this pull request
3 tasks
@slargman
Copy link
Contributor

Everything is looking good to me. I wanted to get your thoughts on a couple of things. I noticed you used a Container component a few times. What do you think about creating a Container component and moving it into the App directory for reuse across the App. I think this could help us maintain a consistent visual style. We could do something similar with the Button component too. You know more about styled components than me, but would we be able to extend or overwrite the styles of these general components for our specific use cases if we did this?

I was thinking that creating a basic component library in App or a shared directory could be useful. Another recommendation I've seen is to establish a set of spacing values used. For example we might have [2px, 5px, 10px, 20px] or something like that and then when we need to use spacing in our components we would just determine if we want XS spacing or M spacing and reference wherever or spacing values are saved.

@slargman
Copy link
Contributor

We can skip all of this for now since the styling aspect can be a bit of a hassle and not as exciting actual development, but just wanted to get your perspective.

@zman811
Copy link
Contributor Author

zman811 commented May 21, 2022

Hey, thanks for looking it over! I use the container a few times, however, it's just the name the styles are different I just use it as a wrapper I could change the names to be different if that would be better? But yeah we can talk more about styles later. I'll go ahead and pull and merge this in a little.

@zman811 zman811 merged commit f7e98ca into main May 22, 2022
@zman811 zman811 deleted the review-skeleton branch May 22, 2022 01:43
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.

Make a skeleton
2 participants