Fixes #1223: Merge our two "sticky headers" on scroll #1274

PedroFonsecaDEV · 2020-11-02T23:00:12Z

Issue This PR Addresses

Type of Change

Bugfix: Change which fixes an issue
New Feature: Change which adds functionality
Documentation Update: Change which improves documentation
UI: Change which improves UI

Description

Fixes: 1223
*ScrollTop and ScrollDown becomes: ScrollAction (We had repeated code for up and down, now we have only one component)
*BackToTop becomes: BackToTopButton - No more coupling with ScrollTop
*Also updated to ES6 the code that I worked on.
*Merge our two "sticky headers" on scroll (#1223)

Checklist

Quality: This PR builds and passes our npm test and works locally
Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

manekenpix · 2020-11-02T23:57:48Z

I really like this, and I have 2 questions:

Could ScrollAction and BackToTopButton be just one component that would scroll up or down depending on a passed prop?
Should ScrollAction and BackToTopButton (or the combination of both if merged) use the same colour that we use in the header?

humphd

This is heading in a great direction, I love this! Before I review the code, some UX issues I notice.

First, there is a pixel difference between the bottom of the nav bar and the post header when they collapse together:

Next, I don't love how we've lost all the padding around the post header.

I think the look is quite different, and not for the better. Can we do something where it stays the way it is until it collapses into the top?

You can't see it great in this GIF, but on my machine it bounces as it changes.

src/frontend/src/components/Banner/Banner.jsx

src/frontend/src/components/Post/Post.jsx

humphd · 2020-11-03T01:03:45Z

src/frontend/src/components/Post/Post.jsx

@@ -100,15 +100,16 @@ const Post = ({ postUrl }) => {
        <Typography variant="h1" title={post.title} id={post.id} className={classes.title}>
          {post.title}
        </Typography>
-        <Typography variant="h3" className={classes.author}>
+        <Typography variant={'p'} className={classes.author}>
+          {' '}


Why not   for these spaces?

humphd · 2020-11-03T01:04:23Z

src/frontend/src/components/ScrollAction/ScrollAction.jsx

+import React from 'react';
+import PropTypes from 'prop-types';
+
+const ScrollAction = (props) => {


const ScrollAction = ({ children }) => {

Sure. Sorry.

agarcia-caicedo

My main problem with the design, is that it uses elevation in both an unexpected way and unlike how we were using it by relying on material design.

Before even interacting with the design, the expectation is as follows (Taken from the material design website)

The app bar either has the highest elevation going over everything else (like Telescope) , or it doesn't and you scroll away (like Github)

When the user does interact with the design, the app-bar already shows that it is elevated higher than both the hero image and the scroll down button, by the combination of using a shadow and by sliding on top of those components

When the user reaches the spot just before the app bar meets the blog card, there is no indication that the card title occupies a higher elevation than the app bar (which it needs to be able to show the title on the app bar). On the contrary, the app bar has a more prominent shadow that gives the impression of a higher elevation.

However, when the user continues scrolling, it reacts in an unexpected way - The card title begins to merge with the app bar, but they visibly don't have the same elevation (or the card title being at a higher elevation) because of the shadow difference

As the user continues to scroll, the unexpected behaviour continues. There isn't an elevation difference between the card title and the card content. So the expectation would be that both would go over the app bar, but they don't. The card title stays on top of the app bar, but the content goes behind.

In the end, this design choice both break the user's expectations based on their previous experiences with material design, and the expectations set up with this design.

Other minor feedback unrelated to elevation. Mostly

The author and the date are a bit too close, which could create an ux issue since the small gap could mean people click on the wrong link.
At a certain width the title of the post cuts of the search button
Before the card title joins the app bar, the sizing it's ok, but once it goes on the app bar, the font size its too big for its space. Rather than increase the padding, (big app bars are not ideal), the font size could be shrunk. Similar how Github does it.

humphd · 2020-11-03T03:17:30Z

@agarcia-caicedo have you got ideas on how to do this in a way that stays within the current design? I'm trying you read your comment above so as to find a way forward.

I really love how this cleans up the view, but retains the post info. But I get what you're saying too.

I guess what I want is my cake and eat it to :)

PedroFonsecaDEV · 2020-11-03T03:51:32Z

I didn't change the font size or spacing, just the padding. So we are seeing the same font and the same size as we see in prod and dev.
But I think the author and date are small too. Let's increase it.
I will improve and fix everything.

PedroFonsecaDEV · 2020-11-09T20:07:26Z

I'm still working on it. The last commit doesn't represent the final work.

PedroFonsecaDEV · 2020-11-11T04:16:31Z

@humphd @manekenpix @cindyledev

All bugs are fixed.
Rebased.
Good to go!

This is just the beginning for a better UX. I hope more people jump into our frontend with more improvements.
Thanks.

PedroFonsecaDEV · 2020-11-11T20:48:25Z

@humphd
Thanks.
Issue:
#1329

manekenpix · 2020-11-11T21:11:14Z

@PedroFonsecaDEV it's better if you rebase, don't merge master into your branch

PedroFonsecaDEV · 2020-11-11T21:47:55Z

PLEASE DON'T MERGE THIS PR.
I WILL START ANOTHER ONE TOMORROW.

Thanks.

manekenpix · 2020-11-11T21:50:07Z

@PedroFonsecaDEV you don't need to submit another PR, just remove the last commit (git reset HEAD~1) and rebase. I'll add the blocked label.

*ScrollTop and ScrollDown becomes: ScrollAction (We had repeated code for up and down, now we have only one component) *BackToTop becomes: BackToTopButton - No more coupling with ScrollTop *Merge our two "sticky headers" on scroll (Seneca-CDOT#1223)

PedroFonsecaDEV · 2020-11-12T23:07:53Z

@humphd @manekenpix

Rebased and good to go.

humphd

@PedroFonsecaDEV looks good. Did you want to try squashing this too, so you only have 1 commit?

vercel bot deployed to Preview November 2, 2020 23:00 View deployment

PedroFonsecaDEV force-pushed the issue1223 branch from 6f7577b to b3bf689 Compare November 2, 2020 23:01

vercel bot deployed to Preview November 2, 2020 23:03 View deployment

manekenpix assigned PedroFonsecaDEV Nov 2, 2020

manekenpix added the area: front-end label Nov 2, 2020

manekenpix added this to In progress in UI and Frontend via automation Nov 2, 2020

manekenpix requested review from cindyledev, humphd, raygervais and c3ho November 3, 2020 00:03

humphd requested a review from agarcia-caicedo November 3, 2020 00:54

humphd requested changes Nov 3, 2020

View reviewed changes

UI and Frontend automation moved this from In progress to Review in progress Nov 3, 2020

humphd reviewed Nov 3, 2020

View reviewed changes

src/frontend/src/components/Banner/Banner.jsx Show resolved Hide resolved

humphd requested changes Nov 3, 2020

View reviewed changes

humphd mentioned this pull request Nov 3, 2020

Long search results need the "Scroll to Top" component #1271

Closed

agarcia-caicedo reviewed Nov 3, 2020

View reviewed changes

humphd mentioned this pull request Nov 6, 2020

Auto hide Nav bar when scrolling down #1297

Closed

PedroFonsecaDEV force-pushed the issue1223 branch from b3bf689 to 7f83f8a Compare November 9, 2020 07:56

vercel bot deployed to Preview November 9, 2020 07:56 View deployment

vercel bot deployed to Preview November 9, 2020 08:29 View deployment

vercel bot deployed to Preview November 9, 2020 20:06 View deployment

PedroFonsecaDEV force-pushed the issue1223 branch from ba3fecd to 413ec2e Compare November 11, 2020 04:04

vercel bot deployed to Preview November 11, 2020 04:04 View deployment

manekenpix requested a review from agarcia-caicedo November 11, 2020 05:00

PedroFonsecaDEV mentioned this pull request Nov 11, 2020

Increase Post Title padding. #1329

Closed

UI and Frontend automation moved this from Review in progress to Reviewer approved Nov 11, 2020

manekenpix previously approved these changes Nov 11, 2020

View reviewed changes

PedroFonsecaDEV dismissed stale reviews from manekenpix and humphd via 8d22130 November 11, 2020 21:09

UI and Frontend automation moved this from Reviewer approved to Review in progress Nov 11, 2020

vercel bot deployed to Preview November 11, 2020 21:09 View deployment

manekenpix added the Blocked Can't do this, until something else is done label Nov 11, 2020

PedroFonsecaDEV added 4 commits November 12, 2020 17:43

FIXES Seneca-CDOT#1223

78eada8

*ScrollTop and ScrollDown becomes: ScrollAction (We had repeated code for up and down, now we have only one component) *BackToTop becomes: BackToTopButton - No more coupling with ScrollTop *Merge our two "sticky headers" on scroll (Seneca-CDOT#1223)

minor fixes before rebase

754c1ee

Fixing mobile height

8516a21

fixing anchors

8d744fb

PedroFonsecaDEV force-pushed the issue1223 branch from 8d22130 to 8d744fb Compare November 12, 2020 22:44

vercel bot deployed to Preview November 12, 2020 22:44 View deployment

Minor adjustment

7c08b94

vercel bot deployed to Preview November 12, 2020 22:59 View deployment

PedroFonsecaDEV requested review from manekenpix and humphd November 12, 2020 23:07

PedroFonsecaDEV removed the Blocked Can't do this, until something else is done label Nov 12, 2020

humphd approved these changes Nov 13, 2020

View reviewed changes

PedroFonsecaDEV requested a review from smilegodly November 13, 2020 02:29

manekenpix approved these changes Nov 13, 2020

View reviewed changes

UI and Frontend automation moved this from Review in progress to Reviewer approved Nov 13, 2020

PedroFonsecaDEV merged commit f4274ab into Seneca-CDOT:master Nov 13, 2020

UI and Frontend automation moved this from Reviewer approved to Done Nov 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1223: Merge our two "sticky headers" on scroll #1274

Fixes #1223: Merge our two "sticky headers" on scroll #1274

PedroFonsecaDEV commented Nov 2, 2020 •

edited by manekenpix

manekenpix commented Nov 2, 2020 •

edited

humphd left a comment

humphd Nov 3, 2020

humphd Nov 3, 2020

PedroFonsecaDEV Nov 3, 2020

agarcia-caicedo left a comment •

edited

humphd commented Nov 3, 2020

PedroFonsecaDEV commented Nov 3, 2020

PedroFonsecaDEV commented Nov 9, 2020

PedroFonsecaDEV commented Nov 11, 2020

PedroFonsecaDEV commented Nov 11, 2020

manekenpix commented Nov 11, 2020

PedroFonsecaDEV commented Nov 11, 2020

manekenpix commented Nov 11, 2020

PedroFonsecaDEV commented Nov 12, 2020

humphd left a comment

Fixes #1223: Merge our two "sticky headers" on scroll #1274

Fixes #1223: Merge our two "sticky headers" on scroll #1274

Conversation

PedroFonsecaDEV commented Nov 2, 2020 • edited by manekenpix

Issue This PR Addresses

Type of Change

Description

Checklist

manekenpix commented Nov 2, 2020 • edited

humphd left a comment

Choose a reason for hiding this comment

humphd Nov 3, 2020

Choose a reason for hiding this comment

humphd Nov 3, 2020

Choose a reason for hiding this comment

PedroFonsecaDEV Nov 3, 2020

Choose a reason for hiding this comment

agarcia-caicedo left a comment • edited

Choose a reason for hiding this comment

Other minor feedback unrelated to elevation. Mostly

humphd commented Nov 3, 2020

PedroFonsecaDEV commented Nov 3, 2020

PedroFonsecaDEV commented Nov 9, 2020

PedroFonsecaDEV commented Nov 11, 2020

PedroFonsecaDEV commented Nov 11, 2020

manekenpix commented Nov 11, 2020

PedroFonsecaDEV commented Nov 11, 2020

manekenpix commented Nov 11, 2020

PedroFonsecaDEV commented Nov 12, 2020

humphd left a comment

Choose a reason for hiding this comment

PedroFonsecaDEV commented Nov 2, 2020 •

edited by manekenpix

manekenpix commented Nov 2, 2020 •

edited

agarcia-caicedo left a comment •

edited