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

Theme update #553

Merged
merged 21 commits into from
May 18, 2019
Merged

Theme update #553

merged 21 commits into from
May 18, 2019

Conversation

elegaanz
Copy link
Member

@elegaanz elegaanz commented Apr 27, 2019

So far:

  • Ligther colors
  • No more border radius
  • Buttons are now always colored
  • Fix weird margin with CWed images
  • Add a border to quotes
  • Start to redesign the post page (according to the Figma mockups)
    • Full-size cover image, with a gradient and the title/subtitle/authors/date over it
    • Like/boost are now between tags and author info
    • Author info has a difference background color to make a difference between the different sections
    • Tags and license are now side-by-side

As I started it, this redesign is not perfectly following what was done on Figma (for instance lighter colors were not planned), but I'm open to discussion if you think Figma mockups should be better respected.

It is available at https://pr-553.joinplu.me/ if you want to see how it looks like. You can login with admin/admin123 to see all the pages.

- Ligther colors
- No more border radius
- Buttons are now always colored
- Start to redesign the post page (according to the Figma mockups)
@elegaanz elegaanz added C: Enhancement New feature or request A: Front-End Related to the front-end S: Incomplete This PR is not complete yet Design UI/UX related issues and PRs Rendering How elements're rendered out for the end user labels Apr 27, 2019
@codecov
Copy link

codecov bot commented Apr 27, 2019

Codecov Report

Merging #553 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master    #553      +/-   ##
=========================================
- Coverage   34.51%   34.5%   -0.01%     
=========================================
  Files          67      67              
  Lines        7872    7876       +4     
  Branches     1890    1890              
=========================================
+ Hits         2717    2718       +1     
- Misses       4394    4398       +4     
+ Partials      761     760       -1

@trwnh
Copy link

trwnh commented Apr 28, 2019

Perhaps main article's max-width should be a bit wider? 40rem is quite narrow. Optimal readability would suggest making it somewhere between 45-75 characters, which can be represented by the ch unit in CSS. So I would propose perhaps making the rule max-width: 70ch instead of max-width: 40rem

@trwnh
Copy link

trwnh commented Apr 28, 2019

Also the main header and main header > img are way too tall, and should each have a max-height of no more than calc(100vh - $heightOfBodyHeader) -- this allows the header to not be bigger than the browser viewport.

@elegaanz
Copy link
Member Author

So… I tried to limit the height of the illustration, but it is impossible to do it without losing the ratio or a part of the image. I don't know what should be done: hide some a portion of the illustration if it is too big, or give the responsibility to author to provide images in reasonable dimensions?

Someone also proposed to have an option that could be enabled to limit the height of the header (by hiding some of the image) if you want to be sure to never have giant images. So that can be a solution too.

@trwnh
Copy link

trwnh commented Apr 29, 2019

I think fitting is better than cropping. You can either accept the ratio loss inherent to cover-fit, or you can cap the max-width of the image/header (in addition to the max-height). That style of header image can be seen in Ghost, for example: https://demo.ghost.io/welcome/

@elegaanz elegaanz changed the title WIP: theme update Theme update Apr 30, 2019
@elegaanz elegaanz added S: Ready for review This PR is ready to be reviewed and removed S: Incomplete This PR is not complete yet labels Apr 30, 2019
It changed because I removed Cargo.lock to handle a merge conflict

I could have updated cargo web too, but it mean I should have re-built
the CI docker image and it was taking forever.
@marek-lach
Copy link
Member

marek-lach commented Apr 30, 2019

This is a great change. I only propose changing the main purple color of links from the current hex code of #7765E3 to something like #887ADE, #8D80DE, or perhaps #9784CB, because on very light backgrounds, it's very hard to see, as shown bellow:
Screenshot 2019-04-30 at 19 46 41

Screenshot 2019-04-30 at 19 53 18

@elegaanz
Copy link
Member Author

elegaanz commented May 1, 2019

@marek-lach Even with another shade of purple the contrast was quite poor, so I made it white, but with underlining when you pass your cursor over it to make you understand it is a link.

@marek-lach
Copy link
Member

marek-lach commented May 5, 2019

Maybe under the About this instance link in the footer, there could also be a Privacy policy link. It would help to fill up the empty space there a little bit.
Screenshot 2019-05-05 at 12 13 02

As per what the `Privacy policy' page itself could contain, just basic stuff could be something like:

If you are browsing this site as a visitor, your IP address is collected by the server for moderation purposes.
As a registered user, you also have to provide your username (which does not have to be your real name), your functional email address and a password, in order to be able to log in, write articles and comment. The content you submit is stored until you delete it.

@elegaanz
Copy link
Member Author

elegaanz commented May 5, 2019

I added it. I just changed a bit your text, because we don't actually collect IP addresses, and I added a part about which cookies we are using.

@marek-lach
Copy link
Member

marek-lach commented May 5, 2019

I added it. I just changed a bit your text, because we don't actually collect IP addresses, and I added a part about which cookies we are using.

Sure 👍

In that case the line As a registered user, you also have to provide your username, doesn't need the also in it.

@trinity-1686a
Copy link
Contributor

I think this should be a default privacy policy only, and should be editable easily by admins (probably in a future pr, this one is supposed to be about design). While we don't store much (no ip, no cookies when not logged in...), most reverse proxy configuration will log at least user ip.

Copy link
Contributor

@trinity-1686a trinity-1686a left a comment

Choose a reason for hiding this comment

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

this would require fixing merge conflict before validation

build.rs Show resolved Hide resolved
static/css/_global.scss Outdated Show resolved Hide resolved
static/css/_variables.scss Outdated Show resolved Hide resolved
elegaanz added 2 commits May 11, 2019 13:25
- Don't watch static/css in build.rs
- Another shade of white
- Remove useless margin rule for error messages
trinity-1686a
trinity-1686a previously approved these changes May 18, 2019
Copy link
Contributor

@trinity-1686a trinity-1686a left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@trinity-1686a trinity-1686a left a comment

Choose a reason for hiding this comment

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

(I just merged master into this to fix conflict)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Front-End Related to the front-end C: Enhancement New feature or request Design UI/UX related issues and PRs Rendering How elements're rendered out for the end user S: Ready for review This PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants