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

CSS changes to author info section #2035

Merged
merged 7 commits into from
Apr 16, 2021
Merged

CSS changes to author info section #2035

merged 7 commits into from
Apr 16, 2021

Conversation

huyxgit
Copy link

@huyxgit huyxgit commented Mar 26, 2021

UPDATED

Closes #1995

  • only show it clear on mouse hover, less distraction on both sides while reading

Screen Shot 2021-03-26 at 1 09 34 PM

  • add marginBottom to make 'sticky author info box' jump earlier (ugly to see both author close together on scrolling)
    Screen Shot 2021-03-26 at 1 15 45 PM

Update:
with Left align in blog content, the info section placed on the right doesn't seem right. It's better on the left of the blog instead:

Screen Shot 2021-04-03 at 1 59 55 PM

Screen Shot 2021-04-03 at 2 02 10 PM

The vertical separator bar is unnecessary and made it looks just unsightly in modern day design, or if you guys wanna keep it, leave it thin and use lighter color.

or remove it and add more margin between blog content (example for Medium)
Screen Shot 2021-04-03 at 1 58 22 PM

ALso about the Navigation menu, no matter what color you use, it will never pops out at first load + banner image in the current design.
My suggestion: Return it back to horizontal top, hidden when scroll down and visible when scroll up.

src/web/src/components/Posts/PostInfo.tsx Show resolved Hide resolved
@@ -9,7 +9,12 @@ const useStyles = makeStyles((theme: Theme) =>
display: 'flex',
flexDirection: 'column',
justifyContent: 'center',
borderLeft: '2.5px solid #707070',
marginBottom: '80vh',
opacity: '0.4',
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea. But let's increase the opacity.
opacity: 0.6 or 0.7.

Copy link
Contributor

@birtony birtony left a comment

Choose a reason for hiding this comment

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

@huynguyez you have an interesting idea, but could you do some research on how similar websites handle this and share some results with us in the PR description, please? I think this would help us come to a team decision. Right now it looks to me like opacity: 0.4 is too much, I would increase it a bit. In terms of the separator, I think we should maybe add some opacity to it and keep it, it helps identify the author info as a separate section from the content of the post. I wouldn't get rid of it. Let's take a look at how others handle such layouts and decide.

birtony
birtony previously approved these changes Apr 7, 2021
Copy link
Contributor

@birtony birtony left a comment

Choose a reason for hiding this comment

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

I personally like the latest update. It seems to be a good compromise for both of your points @PedroFonsecaDEV @huynguyez

Copy link
Contributor

@PedroFonsecaDEV PedroFonsecaDEV left a comment

Choose a reason for hiding this comment

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

I like the changes in the font size.
But I don't think the hover(opacity) is the way to go.
Let's keep without the hover effect.

@chrispinkney
Copy link
Contributor

I like the changes in the font size.
But I don't think the hover(opacity) is the way to go.
Let's keep without the hover effect.

I think I agree with this too. It's a great idea I just don't think it fits well here.

Also needs a rebase as the hyphen changes from Royce aren't included.

@humphd
Copy link
Contributor

humphd commented Apr 9, 2021

Waiting on a rebase + more review, but looking close.

PedroFonsecaDEV
PedroFonsecaDEV previously approved these changes Apr 9, 2021
@PedroFonsecaDEV
Copy link
Contributor

@huynguyez @rjayroso @humphd

No regressions, please.

@PedroFonsecaDEV
Copy link
Contributor

@rjayroso Could you please review the latest commit (adc8198) and check if there is some regression?
The idea is to keep what you did and then add Huy's work to it.

Copy link
Contributor

@PedroFonsecaDEV PedroFonsecaDEV left a comment

Choose a reason for hiding this comment

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

@birtony
Copy link
Contributor

birtony commented Apr 13, 2021

@huynguyez, @rjayroso, @PedroFonsecaDEV let's review this one and merge if it's ready

borderLeft: '2.5px solid #707070',
[theme.breakpoints.up('lg')]: {
width: '22rem',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Re-add the breakpoint to utilize the whitespace on larger screens.

#2026 (comment)

rjayroso
rjayroso previously approved these changes Apr 15, 2021
@humphd
Copy link
Contributor

humphd commented Apr 15, 2021

@huynguyez is this ready for another review?

Copy link
Contributor

@PedroFonsecaDEV PedroFonsecaDEV left a comment

Choose a reason for hiding this comment

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

Screen Shot 2021-04-15 at 8 19 01 PM

Avatar needs to be left-aligned the way we have it on PROD or DEV now. and rebase

@humphd
Copy link
Contributor

humphd commented Apr 16, 2021

@rjayroso an you please review?

@huyxgit
Copy link
Author

huyxgit commented Apr 16, 2021

@rjayroso an you please review?

@humphd I think I will merge it anyway because @rjayroso approved this yesterday, the latest commit just changed a bit of marginLeft.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Sounds good.

@rjayroso, file a follow-up if necessary.

@huyxgit huyxgit merged commit cc88fe7 into Seneca-CDOT:master Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tweaking author info section
6 participants