Skip to content

Conversation

@adlius
Copy link
Collaborator

@adlius adlius commented Nov 10, 2017

Purpose

Currently, the color contrast for support email link on ECSarxiv and EarthArxiv error pages are not enough. This PR fix the issue.

Summary of Changes

Change the component's CSS class to 'prerpint-header' and 'preprint-hearder-error'.
Added a style file to the component to add padding and bottom margin.

Side Effects / Testing Notes

The error page for OSF preprint would also be changed to using the background image on the landing page, which should be more consistent.

Ticket

https://openscience.atlassian.net/browse/EOSF-881

Reviewer Checklist

  • meets requirements
  • easy to understand
  • DRY
  • testable and includes test(s)
  • changes described in CHANGELOG.md

@coveralls
Copy link

Coverage Status

Coverage remained the same at 25.453% when pulling fd66612 on adlius:css-contrast into a03b964 on CenterForOpenScience:develop.

Copy link
Member

@jamescdavis jamescdavis left a comment

Choose a reason for hiding this comment

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

Please describe this change in the [unreleased] section of CHANGELOG.md

@adlius
Copy link
Collaborator Author

adlius commented Nov 30, 2017

CHANGELOG.md is modified.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 25.489% when pulling 8dfc46d on adlius:css-contrast into a03b964 on CenterForOpenScience:develop.

@jamescdavis jamescdavis changed the base branch from develop to release/post-review-actions December 18, 2017 14:58
@hmoco
Copy link
Contributor

hmoco commented Dec 18, 2017

This looks good. One small concern, and this seems to be intended, but if so we should be more explicit about it. Replacing preprint-error-page does not allow brands to change their color here. The PR description implies that that is intended. However, preprint-error-page is still a class in the https://github.com/CenterForOpenScience/ember-osf-preprints/blob/develop/app/styles/brands/_brand.scss file. My opinion is that if we intend to remove that option, the class should also be removed from the customized .css brand files. Opinions? @jamescdavis @adlius

@adlius
Copy link
Collaborator Author

adlius commented Dec 18, 2017

You're right. The side effect is that the brands cannot customize how they want the error page to look like without also changing the header elements. This should be more favorable as it results in more consistent looks. If that is the case, I can go and remove that from the CSS files for brands.

@hmoco
Copy link
Contributor

hmoco commented Dec 18, 2017

I honestly prefer the button solution, to keep brands' themes consistent. However, that is inconsistent UI with our other support links, inconsistent UX to what buttons normally do versus anchor tags, and it might look ugly. Also, its not like all our brands have great themes (cough cough agrixiv). So, I'm happy with just removing the CSS pertinent to error pages from those files and the brand template.

@jamescdavis jamescdavis merged commit 48d172b into CenterForOpenScience:release/post-review-actions Dec 19, 2017
@laurenbarker laurenbarker added this to the 0.117.0 milestone Jan 22, 2018
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