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

main css #39

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

main css #39

wants to merge 4 commits into from

Conversation

lacrivella
Copy link
Contributor

…w team. Minor css adjustments on the buttons in Welcome.css

For an example of how to fill this template out, see this Pull Request.

Description

Added a div with a class name of main into App.js to allow us to have the same background throughout our app. Changed it from black to whitesmoke/light gray based on our discussion as a team---basically switched our font and background div colors. Minor css edits on the button. Updated h1 and h2 in Welcome.js to now be a h1 with a span.

Update passes Accessibility Insights extension.

Related Issue

Acceptance Criteria

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring
💯 Add tests
🔗 Update dependencies
📜 Docs

Updates

Before

Screen Shot 2020-09-18 at 1 50 09 PM

After

Screen Shot 2020-09-23 at 4 29 29 PM

Testing Steps / QA Criteria

…w team. Minor css adjustments on the buttons in Welcome.css
Copy link
Contributor

@fab33150 fab33150 left a comment

Choose a reason for hiding this comment

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

Nice! The white is more pleasant touch

Copy link
Contributor

@skillitzimberg skillitzimberg left a comment

Choose a reason for hiding this comment

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

These changes make the app look really good. Super slick. (I'm a fan of transparency. 😄 )

It looks like there are a few different issues/components being addressed in one branch, which could get really confusing fast so make sure everyone's clear when you start to merge your branches.

But the solutions made for each issue/component are, as usual, as simple as possible and no simpler. Great job!

Copy link
Contributor

@yvesgurcan yvesgurcan left a comment

Choose a reason for hiding this comment

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

I'm glad to see the application coming together! I would recommend to stay consistent with the unit you use to set padding and margins. It seems that some rules use percentages while others use rems. Try to convert the % to rems. Using % makes sense for width and height properties, however.

}

.not-so-soon {
color: green;
@media only screen and (min-width: 736px) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay! Media queries.

background-color: rgba(0, 0, 0, 0.25);
}
/*
NEEDS TO BE REMOVED TO ANOTHER CSS FILE.
Copy link
Contributor

Choose a reason for hiding this comment

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

There's nothing like today to get this done 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a head's up . . .
This gets handled in @lacrivella 's modal css branch.

.additem__form {
width: 90%;
margin: 0;
padding: 5%;
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to stick to one kind of unit. What would 5% be in rem?

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.

6 participants