Add GitHub information in mobile view. #2716

TueeNguyen · 2022-01-23T00:46:58Z

Issue This PR Addresses

Fixes #2373
This continues the work from #2491

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

This PR continues the work from PR #2373 Update on github information on mobile view, need update on the titles #2491
Added borderBottom for accordion/header
Added expand icon to hint users
Changed a bit CSS to make publish data and expand icon look better
Changed flex display to row, wrap for repos information

Mobile device

2022-01-22.19-51-51.mp4

Tablet

2022-01-22.19-51-18.mp4

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)

gitpod-io · 2022-01-23T00:47:00Z

humphd · 2022-01-23T14:21:47Z

@TueeNguyen can you assign some reviewers to this please?

AmasiaNalbandian

This actually looks REALLY great! I just had a few questions/ and one issue I found:

When you expand an accordion, and I scroll down, then I go back up to the post, the accordion is still open - do we want this?
personally, i think its hard to figure out how to close the accordion... a suggestion could be to create a chevron that is at the bottom of all the info once its opened, that we can click to close? Maybe the feature is more friendly if we do the same to open. I've attached my version of paint for this.
When I click on the chevron pointing down, I am sometimes also copying the link to the blog... we should try to avoid the overlap in the click area.

Kevan-Y

This looks great, finally, we can have the GitHub status on mobile.

Found some CSS to fix:
Avatar image cut off for smaller screen size.

HyperTHD · 2022-01-25T03:00:43Z

Once this is merged in, we need to start consolidating all this new github info so we don't bloat the posts folder. I wanna see if it's possible to make a POC for a custom hook for this issue at some point this week. Will discuss in more detail whenever

TueeNguyen · 2022-01-25T19:31:06Z

@AmasiaNalbandian, I believe I've fixed all issues you pointed out

TueeNguyen · 2022-01-25T19:32:19Z

Once this is merged in, we need to start consolidating all this new github info so we don't bloat the posts folder. I wanna see if it's possible to make a POC for a custom hook for this issue at some point this week. Will discuss in more detail whenever

Definitely, in my recent commit, I also added a function to keep track of scrolling, it could be destructure as a hook, we'll take a look

AmasiaNalbandian

All of UI for me is good to go now! We just have a few bugs to fix now. Also when we view info and then scroll up, but come back down, the info is open. I think you resolved this for only one direction. I'll leave it up to you whether this is how we want it. I just think we should make both cases consistent (keep info open no matter scroll, or close if you go to another post)

src/web/src/components/Posts/GitHubInfoMobile.tsx

src/web/src/components/Posts/Post.tsx

AmasiaNalbandian · 2022-01-25T21:58:34Z

src/web/src/components/Posts/Post.tsx

+                  </a>
+                  <ShareButton url={post.url} />
+                </h1>
+                <ExpandIcon small={false} expandHeader={expandHeader} />


What's happening now is that when I click the copylink, it is expanding the information section.

This icon needs to go into the H1 div, I believe. I inspected the elements, and the H1 is the div that holds the date, and the copy icon. If we add the chevron in this div, they should not overlap one another. We might have to remove the padding from the copy icon(its massive...). Heres a gif to show the tree::

AmasiaNalbandian · 2022-01-25T22:02:54Z

src/web/src/components/Posts/ExpandIcon.tsx

+      {expandHeader ? (
+        <ExpandLessIcon className={classes.smallIcon} />
+      ) : (
+        <ExpandMoreIcon className={classes.smallIcon} />


The expand more icon doesn't show up when we have the info section collapse, so we're only seeing it when we close it and see a quick flash of it

Can you decide if we need it and use it, or remove the code for it please

I'll remove the condition that toggles the big icon. Maybe we don't need it

Kevan-Y · 2022-01-25T23:50:27Z

src/web/src/components/Posts/PostAvatar.tsx

+        width: '2.5em',
+        height: '2.5em',


Do we need to restate width and height? If you removed it, I think it will inherit from the previous breakpoint.

Yeah that's what I noticed too, I think it's ok to remove it

TueeNguyen · 2022-01-26T02:11:50Z

This is how it currently behaves

2022-01-25.21-05-29.mp4

Kevan-Y

LGTM, great feature👍

HyperTHD

I have a small nit-pick with the accordion being hard to view in light mode, contrast being the root cause, but functionally speaking, everything appears to be in order.

TueeNguyen · 2022-01-27T03:06:36Z

I have a small nit-pick with the accordion being hard to view in light mode, contrast being the root cause, but functionally speaking, everything appears to be in order.

Right, I should have added the color based on the theme

I'd love to finish the design but this PR is already a very long one. I'll file an issue for it if that's ok @HyperTHD

DukeManh

@TueeNguyen, great work, let's continue to improve in later PRs.

src/web/src/components/Posts/Post.tsx

humphd · 2022-01-27T23:42:20Z

@TueeNguyen let's tie this up and get it landed soon, it's been open for so long.

src/web/src/components/Posts/Post.tsx

AmasiaNalbandian

We can clean up the code as Duke suggested, and then let's ship this. It's excellent work! Also can we unlink the issue #2708 I wrote more info about it on the issue, however this PR does not address that one. We can tackle that one in 2.7!

- Changed repos display to row, added border bottom for header, added hint icon for accordion - Fixed clicking on expand icon copies link to blog post - Added expand icons in the bottom of the github info - Added function to auto close accordion when scroll pass post - Added css for mobile device < 375 px width

TueeNguyen · 2022-01-28T18:58:58Z

Noted all your comments, I've unlinked issue #2708

vercel bot deployed to Preview January 23, 2022 00:47 View deployment

TueeNguyen force-pushed the issue-2373-5 branch from 5fd9f88 to 3440918 Compare January 23, 2022 00:55

vercel bot deployed to Preview January 23, 2022 00:55 View deployment

TueeNguyen self-assigned this Jan 23, 2022

TueeNguyen requested review from dbelokon, Kevan-Y and AmasiaNalbandian January 23, 2022 15:40

TueeNguyen mentioned this pull request Jan 23, 2022

Mobile UX/UI accordion behavior improvement #2708

Closed

AmasiaNalbandian requested changes Jan 24, 2022

View reviewed changes

Kevan-Y requested changes Jan 24, 2022

View reviewed changes

vercel bot deployed to Preview January 25, 2022 19:27 View deployment

vercel bot deployed to Preview January 25, 2022 20:10 View deployment

AmasiaNalbandian requested changes Jan 25, 2022

View reviewed changes

Kevan-Y reviewed Jan 25, 2022

View reviewed changes

vercel bot deployed to Preview January 26, 2022 02:00 View deployment

vercel bot deployed to Preview January 26, 2022 02:01 View deployment

humphd requested review from Kevan-Y and AmasiaNalbandian January 26, 2022 15:01

Kevan-Y previously approved these changes Jan 26, 2022

View reviewed changes

HyperTHD previously approved these changes Jan 27, 2022

View reviewed changes

DukeManh approved these changes Jan 27, 2022

View reviewed changes

DukeManh requested changes Jan 27, 2022

View reviewed changes

src/web/src/components/Posts/Post.tsx Show resolved Hide resolved

DukeManh added this to the 2.6 Release milestone Jan 27, 2022

TueeNguyen requested a review from DukeManh January 28, 2022 00:07

DukeManh requested changes Jan 28, 2022

View reviewed changes

src/web/src/components/Posts/Post.tsx Show resolved Hide resolved

src/web/src/components/Posts/Post.tsx Show resolved Hide resolved

src/web/src/components/Posts/Post.tsx Outdated Show resolved Hide resolved

AmasiaNalbandian previously approved these changes Jan 28, 2022

View reviewed changes

TueeNguyen dismissed stale reviews from AmasiaNalbandian, HyperTHD, and Kevan-Y via a7c8d68 January 28, 2022 17:41

TueeNguyen force-pushed the issue-2373-5 branch from f5b22b7 to a7c8d68 Compare January 28, 2022 17:41

vercel bot deployed to Preview January 28, 2022 17:41 View deployment

TueeNguyen requested review from DukeManh, HyperTHD, Kevan-Y and AmasiaNalbandian January 28, 2022 18:57

DukeManh approved these changes Jan 28, 2022

View reviewed changes

Kevan-Y approved these changes Jan 28, 2022

View reviewed changes

cindyorangis approved these changes Jan 28, 2022

View reviewed changes

TueeNguyen merged commit d7062de into Seneca-CDOT:master Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GitHub information in mobile view. #2716

Add GitHub information in mobile view. #2716

TueeNguyen commented Jan 23, 2022 •

edited

Loading

gitpod-io bot commented Jan 23, 2022

humphd commented Jan 23, 2022

AmasiaNalbandian left a comment

Kevan-Y left a comment •

edited

Loading

HyperTHD commented Jan 25, 2022

TueeNguyen commented Jan 25, 2022 •

edited

Loading

TueeNguyen commented Jan 25, 2022

AmasiaNalbandian left a comment

AmasiaNalbandian Jan 25, 2022 •

edited

Loading

AmasiaNalbandian Jan 25, 2022

TueeNguyen Jan 25, 2022

Kevan-Y Jan 25, 2022

TueeNguyen Jan 26, 2022

TueeNguyen commented Jan 26, 2022

Kevan-Y left a comment

HyperTHD left a comment

TueeNguyen commented Jan 27, 2022 •

edited

Loading

DukeManh left a comment

humphd commented Jan 27, 2022

AmasiaNalbandian left a comment

TueeNguyen commented Jan 28, 2022

Add GitHub information in mobile view. #2716

Add GitHub information in mobile view. #2716

Conversation

TueeNguyen commented Jan 23, 2022 • edited Loading

Issue This PR Addresses

Type of Change

Description

Mobile device

Tablet

Checklist

gitpod-io bot commented Jan 23, 2022

humphd commented Jan 23, 2022

AmasiaNalbandian left a comment

Choose a reason for hiding this comment

Kevan-Y left a comment • edited Loading

Choose a reason for hiding this comment

HyperTHD commented Jan 25, 2022

TueeNguyen commented Jan 25, 2022 • edited Loading

TueeNguyen commented Jan 25, 2022

AmasiaNalbandian left a comment

Choose a reason for hiding this comment

AmasiaNalbandian Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

AmasiaNalbandian Jan 25, 2022

Choose a reason for hiding this comment

TueeNguyen Jan 25, 2022

Choose a reason for hiding this comment

Kevan-Y Jan 25, 2022

Choose a reason for hiding this comment

TueeNguyen Jan 26, 2022

Choose a reason for hiding this comment

TueeNguyen commented Jan 26, 2022

Kevan-Y left a comment

Choose a reason for hiding this comment

HyperTHD left a comment

Choose a reason for hiding this comment

TueeNguyen commented Jan 27, 2022 • edited Loading

DukeManh left a comment

Choose a reason for hiding this comment

humphd commented Jan 27, 2022

AmasiaNalbandian left a comment

Choose a reason for hiding this comment

TueeNguyen commented Jan 28, 2022

TueeNguyen commented Jan 23, 2022 •

edited

Loading

Kevan-Y left a comment •

edited

Loading

TueeNguyen commented Jan 25, 2022 •

edited

Loading

AmasiaNalbandian Jan 25, 2022 •

edited

Loading

TueeNguyen commented Jan 27, 2022 •

edited

Loading