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

Changes site's main content background color from white to #F5F5F5 #163

Merged
merged 5 commits into from
May 28, 2020
Merged

Changes site's main content background color from white to #F5F5F5 #163

merged 5 commits into from
May 28, 2020

Conversation

JackieBinya
Copy link
Contributor

@JackieBinya JackieBinya commented May 13, 2020

Fixes

Fixes #162 by @annatuma

Description

  • Changes sites' main content background color from white to #F5F5F5 as stipulated in the design mocks

Screenshots

  • After I changed the site's main content background color to #F5F5F5. I noticed several bugs in the site's appearance. Further details about bugs are described below:
  1. White gap between <main> and footer elements: To resolve this issue I removed the margin-top of 5rem and gave the <main> a padding-bottom of 5rem so as to preserve the original look of the site.

001

  1. Selected language no longer visible in the locale-chooser component: Resolved

000

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the master branch of the repository.
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@JackieBinya JackieBinya requested review from a team, obulat and brenoferreira and removed request for a team May 13, 2020 09:50
@JackieBinya
Copy link
Contributor Author

Final look

cc

@JackieBinya JackieBinya changed the title Changes sites' main content background color from white to #F5F5F5 Changes site's main content background color from white to #F5F5F5 May 18, 2020
@obulat
Copy link
Contributor

obulat commented May 19, 2020

Looks great, Jackie!
I noticed a couple of points that are different from the designs:

  • In the designs, the steps that have not been interacted with have grey background, and the current step and the step before that have white background. Here, all the steps' background is white.

  • The button text on the current step is grey and bold. Here, it's white.

  • Feedback link doesn't have an 'external link' icon: External link

  • Embeddable license code background on the licenseUseCard on the mockup is also white.

@JackieBinya
Copy link
Contributor Author

Looks great, Jackie!
I noticed a couple of points that are different from the designs:

  • In the designs, the steps that have not been interacted with have grey background, and the current step and the step before that have white background. Here, all the steps' background is white.
  • The button text on the current step is grey and bold. Here, it's white.
  • Feedback link doesn't have an 'external link' icon: External link
  • Embeddable license code background on the licenseUseCard on the mockup is also white.

@obulat noted. I will make further commits and try to resolve the issues you identified.

@obulat
Copy link
Contributor

obulat commented May 24, 2020

  • The button text on the current step is grey and bold. Here, it's white.

I think this can be addressed later, after this PR is merged, using Vocabulary styles.

  • Feedback link doesn't have an 'external link' icon: External link

I added the icon in #169, Jackie, so you don't need to worry about it.

}
.locale-chooser .select:not(.is-multiple):not(.is-loading)::after {
z-index: 0;
z-index: 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I've reworked positioning for locale chooser in another PR, so you can leave it as it was before.
I just noticed that github shows me that I haven't reviewed your PR, although I did leave a reply. It doesn't let me click 'request changes' if I don't add any reply now. So, I added this note to be able to click 'request changes' :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok great will try to clear off the remaining issue just now.

@kgodey
Copy link
Member

kgodey commented May 26, 2020

Please make sure to re-request review once you push new changes. It looks like there's some merge conflicts here that need to be resolved, though.

@JackieBinya
Copy link
Contributor Author

@obulat may you please review this PR. I implemented all the remaining changes for the styling.

Copy link
Contributor

@obulat obulat left a comment

Choose a reason for hiding this comment

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

Hi, Jackie, I'm sorry I couldn't review the PR today. I tried, but there are some merge conflicts that need resolving. Could you maybe rebase your fork? Here are the instructions, if you need them: https://stackoverflow.com/questions/7244321/how-do-i-update-a-github-forked-repository

src/App.vue Outdated Show resolved Hide resolved
- adds <main> tag to apps layout gives it class .main & bulma class .has-background-light
- gives .main padding-bottom of 5rem
- and removed margin top of 5rem from footer so as to get rid of white space between footer and main
- Changes font weight from 500 to bold
- Edits font color of next button when disabled
@JackieBinya
Copy link
Contributor Author

JackieBinya commented May 28, 2020

Hello @obulat, I did the rebase then proceeded to change #app background-color as you suggested. All the changes have been implemented so far but I am not sure if the background color #f5f5f5 I used for #app. When I cross-checked the mocks the suggested color for the background is #e5e5e5 but visually to me it was too dark, darker than the background in the mocks.
So do let me know what you think.

@obulat
Copy link
Contributor

obulat commented May 28, 2020

This happened to me, too, Jackie. The color you mentioned is the background of Figma itself To see the mockup background, you have to click on its frame. It's vocabulary light gray, which is #d8d8d8

@obulat
Copy link
Contributor

obulat commented May 28, 2020

It's vocabulary light gray, which is #d8d8d8.

Sorry, Jackie, it is not light gray, but lighter gray, #f5f5f5, so you've used the correct color.

@obulat obulat closed this May 28, 2020
@obulat obulat reopened this May 28, 2020
@obulat
Copy link
Contributor

obulat commented May 28, 2020

Looks great, thank you, Jackie! Merging.

@obulat obulat merged commit 53a0b8d into creativecommons:master May 28, 2020
@obulat
Copy link
Contributor

obulat commented Jun 1, 2020

Hey @JackieBinya, thanks for your contribution! We were impressed by the quality of your work, and would like to recognize that. As such, we have assigned you to this PR, and applied the “Great Contribution” label.

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.

Header and background updates
3 participants