#2373 Update on github information on mobile view, need update on the titles #2491

irenejoeunpark · 2021-11-18T20:24:59Z

Issue This PR Addresses

#2373 More change is needed after some feedbacks on styles

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 contains an update on the UI for mobile view, to include GitHub information on each post.
When the user clicks on the GitHub icon, it expands the GitHub information of the post.

Need feedback on the icon, design, and the idea of using Accordion.
Need to fix the title and the author name position on mobile view - need some advice on it.

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 -> Not a functional change, no need of functional tests to be added.
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) -> UI change, not applicable

gitpod-io · 2021-11-18T20:25:02Z

src/web/src/components/Posts/Commits.tsx

humphd · 2021-11-18T22:56:01Z

I think the idea is good. The details need some work, but having it expand vertically to reveal the GitHub info is interesting. We need to find a way to use the space more efficiently on mobile (not jammed to the right). Also, I'm not sure where the GitHub icon should go, or what it should look like. It might need some text to indicate there is More... somehow.

I would focus on figuring out how to arrange this same info in a smaller space efficiently on mobile (e.g., what's in the revealed area).

DukeManh

This is awesome.

I'm just thinking about whether we should display the github info at the bottom of each post by default vs make users click a button and see a layout shift.

src/web/src/components/Posts/GitHubMobile.tsx

src/web/src/components/Posts/Issues.tsx

src/web/src/components/Posts/PullRequests.tsx

irenejoeunpark · 2021-11-20T17:20:11Z

I think the idea is good. The details need some work, but having it expand vertically to reveal the GitHub info is interesting. We need to find a way to use the space more efficiently on mobile (not jammed to the right). Also, I'm not sure where the GitHub icon should go, or what it should look like. It might need some text to indicate there is More... somehow.

I would focus on figuring out how to arrange this same info in a smaller space efficiently on mobile (e.g., what's in the revealed area).

I found another method for collapsable, called Colllapse in material-UI, here is an example of the usage.
https://codesandbox.io/s/77r47?file=/src/App.js:0-41
Should I use this to collapse the info horizontally?
I am also thinking that no one would notice that the GitHub icon is for expanding information, and people would rarely click it.
So, I am changing it to ... which seems more reasonable and could lead more people to click it.

humphd

I think this needs more UX work. Here's the comparison of what we have now (left) with what this does (right):

I think adding another ... clutters the view. Here's my suggestion instead:

Make the entire header (avatar logo, title, name, etc) a single clickable area
If there is GitHub data to show, add a small GitHub icon to the right of the person's name, which would indicate that there is GitHub data to reveal
Touching the header would open the header to a larger size

The current layout of the GitHub data isn't going to work on mobile:

We need to use the entire width somehow. cc @Meneguini, @PedroFonsecaDEV for ideas.

irenejoeunpark · 2021-11-23T20:14:53Z

I think this needs more UX work. Here's the comparison of what we have now (left) with what this does (right):

I think adding another ... clutters the view. Here's my suggestion instead:

Make the entire header (avatar logo, title, name, etc) a single clickable area

If there is GitHub data to show, add a small GitHub icon to the right of the person's name, which would indicate that there is GitHub data to reveal

Touching the header would open the header to a larger size

The current layout of the GitHub data isn't going to work on mobile:

We need to use the entire width somehow. cc @Meneguini, @PedroFonsecaDEV for ideas.

It seems like a good design. I will try to make the whole title container a clickable area and add a GitHub icon if there's any GitHub link attached.

irenejoeunpark · 2021-11-23T23:12:02Z

I think this needs more UX work. Here's the comparison of what we have now (left) with what this does (right):

I think adding another ... clutters the view. Here's my suggestion instead:

Make the entire header (avatar logo, title, name, etc) a single clickable area

If there is GitHub data to show, add a small GitHub icon to the right of the person's name, which would indicate that there is GitHub data to reveal

Touching the header would open the header to a larger size

The current layout of the GitHub data isn't going to work on mobile:

We need to use the entire width somehow. cc @Meneguini, @PedroFonsecaDEV for ideas.

@humphd I have revised the code to make the title container clickable, and show the GitHub icon beside the author name when there is any GitHub info available.

Now I faced another challenge, which the title container, which has been pinned to the top before I make the change, is now scrolled down with the post content. I tried a few different things, but it never gets pinned on top even though I only updated it to put the whole thing in the AccordionSummary. <- might be related to the issue?
I will ask for other reviews in the slack.

humphd · 2021-12-08T19:08:04Z

Excellent, thank you. Can you rebase to pick up the changes to src/web/src/components/Posts/*?

…o issue#2373

humphd · 2021-12-16T22:10:51Z

This isn't perfect, but it's better. The mechanism of showing/hiding the info is working well. I'm not sure about the font size change on the header text. And the layout of the inner contents are still not right for mobile.

I'll be interested to see further improvements and hear more from other reviewers.

DukeManh

This is getting closer. I left some reviews.

DukeManh · 2021-12-17T03:29:33Z

src/web/src/components/Posts/Users.tsx

@@ -15,6 +15,10 @@ const useStyles = makeStyles((theme: Theme) =>
      lineHeight: 'normal',
      fontSize: '1.2rem',
    },
+    GitHubMobile: {


This class isn't needed.

DukeManh · 2021-12-17T03:43:53Z

src/web/src/components/Posts/GItHubInfoMobile.tsx

+
+  return (
+    <div>
+      <ListSubheader className={classes.root}>


Can you make this <ListSubheader component="div"> and the same to every ListSubheader you add? We have a number of wrong uses of ListSubheader throughout the tree because we like its default style. By default it renders a <li>, li is meant for lists, we don't want to end up having nested <li>. I have fixed some <ListSubheader component="div"> in #2594 but to avoid this in the future I'll file an issue to replace all usage of ListSubheader with a different component while maintaining the same style.

DukeManh · 2021-12-17T03:45:27Z

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

-                  <time dateTime={post.updated}>{`${formatPublishedDate(post.updated)}`}</time>
-                </a>
+      {!desktop && (
+        <>


This fragment isn't needed.

DukeManh · 2021-12-17T03:48:32Z

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

+            </AccordionDetails>
+          </Accordion>
+        </>
+      )}
      {desktop && (


It'll be nice if y can combine these two expression into something like desktop ? <A/> : <B/>

DukeManh · 2021-12-17T03:50:13Z

src/web/src/components/Posts/Repos.tsx

@@ -15,6 +15,11 @@ const useStyles = makeStyles((theme: Theme) =>
      wordWrap: 'break-word',
      lineHeight: 'normal',
    },
+    GitHubMobile: {


This class isn't needed.

DukeManh · 2021-12-17T04:10:55Z

src/web/src/components/Posts/GItHubInfoMobile.tsx

+
+const GitHubInfoMobile = ({ ghUrls }: Props) => {
+  const classes = useStyles();
+  const { repos, issues, pullRequests, commits, users } = filterGitHubUrls(ghUrls);


let's cache the result of this function call with useMemo so that it doesn't get called every time the Accordion is open.

Suggested change

const { repos, issues, pullRequests, commits, users } = filterGitHubUrls(ghUrls);

const { repos, issues, pullRequests, commits, users } = useMemo(

() => filterGitHubUrls(ghUrls),

[ghUrls]

);

Can you do the same for GithubInfo?

DukeManh · 2021-12-17T04:21:09Z

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

+        padding: '0 0 0 1rem',
+      },
+    },
+    gitHubIcon: {


You don't use a githubIcon anymore right.

DukeManh · 2021-12-17T04:40:40Z

One area of the inner layout to be improved is the repos list to take up some of the horizontal space. If it looks clustered to you, feel free to design around and add visual separators between them :).

Column	Row

HyperTHD · 2021-12-18T02:33:53Z

One area of the inner layout to be improved is the repos list to take up some of the horizontal space. If it looks clustered to you, feel free to design around and add visual separators between them :).

Column Row

I'm definitely leaning more towards the row style, given a suitable max-width ofc. Column view makes it easier to view the list of repos, at the expense of a much longer height and unused width space being used.

humphd · 2021-12-18T14:46:57Z

Using row vs. column flex with this breakpoint could be the answer, I agree. That looks great.

sirinoks · 2022-01-19T18:37:16Z

We are closing this PR and making our own branch to revert some changes and work on the issue. I new PR will be opened once it is ready.

- Changed repos display to row, added border bottom for header, added hint icon for accordion

- 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

@irenejoeunpark

- Initial work from @irenejoeunpark with PR #2491 (#2716) - 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 Co-authored-by: Francesco Menghi <53121061+menghif@users.noreply.github.com>

irenejoeunpark added 3 commits November 17, 2021 15:47

Added non-desktop github info

be536a8

Replaced text to github icon

6722c01

date position modified

927c8be

vercel bot deployed to Preview November 18, 2021 20:25 View deployment

humphd requested review from DukeManh, HyperTHD, menghif, Andrewnt219, manekenpix and cindyorangis November 18, 2021 20:48

DukeManh reviewed Nov 18, 2021

View reviewed changes

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

DukeManh requested changes Nov 18, 2021

View reviewed changes

irenejoeunpark added 2 commits November 20, 2021 12:27

Removed unused properties

ba6eaed

Removed unused properties

181f124

vercel bot deployed to Preview November 20, 2021 19:55 View deployment

Deleted repeated function and reused GitHubInfo

dfed851

vercel bot deployed to Preview November 22, 2021 08:32 View deployment

irenejoeunpark requested a review from DukeManh November 22, 2021 08:34

AmasiaNalbandian assigned irenejoeunpark Nov 22, 2021

humphd requested changes Nov 23, 2021

View reviewed changes

irenejoeunpark added 3 commits November 23, 2021 15:57

Transformed whole title container clickable

f8b55da

Transformed whole title container clickable

c3dec52

Removed GitHubMobile and reused GitHubInfo

dc55bff

vercel bot deployed to Preview November 23, 2021 22:58 View deployment

Merge branch 'master' into issue#2373

7928dd8

Removed unused variable

f026740

vercel bot deployed to Preview December 13, 2021 17:35 View deployment

Merge branch 'master' of https://github.com/Seneca-CDOT/telescope int…

2985d9a

…o issue#2373

vercel bot deployed to Preview December 16, 2021 20:00 View deployment

Added non-desktop github info

86dca2e

vercel bot had a problem deploying to Preview December 16, 2021 20:29 Failure

vercel bot deployed to Preview December 16, 2021 22:03 View deployment

DukeManh requested changes Dec 17, 2021

View reviewed changes

humphd assigned sirinoks, menghif and TueeNguyen and unassigned irenejoeunpark Jan 13, 2022

humphd added this to the 2.5 Release milestone Jan 13, 2022

menghif removed their request for review January 15, 2022 15:42

Merge branch 'master' into issue#2373

0486f87

vercel bot deployed to Preview January 17, 2022 22:03 View deployment

sirinoks closed this Jan 19, 2022

TueeNguyen pushed a commit to TueeNguyen/telescope that referenced this pull request Jan 23, 2022

- Initial work from @irenejoeunpark with PR Seneca-CDOT#2491

b6447b6

- Changed repos display to row, added border bottom for header, added hint icon for accordion

TueeNguyen pushed a commit to TueeNguyen/telescope that referenced this pull request Jan 23, 2022

- Initial work from @irenejoeunpark with PR Seneca-CDOT#2491

5fd9f88

- Changed repos display to row, added border bottom for header, added hint icon for accordion

TueeNguyen mentioned this pull request Jan 23, 2022

Add GitHub information in mobile view. #2716

Merged

8 tasks

TueeNguyen pushed a commit to TueeNguyen/telescope that referenced this pull request Jan 23, 2022

- Initial work from @irenejoeunpark with PR Seneca-CDOT#2491

3440918

- Changed repos display to row, added border bottom for header, added hint icon for accordion

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2373 Update on github information on mobile view, need update on the titles #2491

#2373 Update on github information on mobile view, need update on the titles #2491

irenejoeunpark commented Nov 18, 2021 •

edited by gitpod-io bot

Loading

gitpod-io bot commented Nov 18, 2021

humphd commented Nov 18, 2021

DukeManh left a comment •

edited

Loading

irenejoeunpark commented Nov 20, 2021

humphd left a comment

irenejoeunpark commented Nov 23, 2021

irenejoeunpark commented Nov 23, 2021

humphd commented Dec 8, 2021

humphd commented Dec 16, 2021

DukeManh left a comment

DukeManh Dec 17, 2021

DukeManh Dec 17, 2021

DukeManh Dec 17, 2021

DukeManh Dec 17, 2021

DukeManh Dec 17, 2021

DukeManh Dec 17, 2021

DukeManh Dec 17, 2021

DukeManh commented Dec 17, 2021

HyperTHD commented Dec 18, 2021

humphd commented Dec 18, 2021

sirinoks commented Jan 19, 2022

-  const { repos, issues, pullRequests, commits, users } = filterGitHubUrls(ghUrls);
+  const { repos, issues, pullRequests, commits, users } = useMemo(
+    () => filterGitHubUrls(ghUrls),
+    [ghUrls]
+  );

#2373 Update on github information on mobile view, need update on the titles #2491

#2373 Update on github information on mobile view, need update on the titles #2491

Conversation

irenejoeunpark commented Nov 18, 2021 • edited by gitpod-io bot Loading

Issue This PR Addresses

Type of Change

Description

Checklist

gitpod-io bot commented Nov 18, 2021

humphd commented Nov 18, 2021

DukeManh left a comment • edited Loading

Choose a reason for hiding this comment

irenejoeunpark commented Nov 20, 2021

humphd left a comment

Choose a reason for hiding this comment

irenejoeunpark commented Nov 23, 2021

irenejoeunpark commented Nov 23, 2021

humphd commented Dec 8, 2021

humphd commented Dec 16, 2021

DukeManh left a comment

Choose a reason for hiding this comment

DukeManh Dec 17, 2021

Choose a reason for hiding this comment

DukeManh Dec 17, 2021

Choose a reason for hiding this comment

DukeManh Dec 17, 2021

Choose a reason for hiding this comment

DukeManh Dec 17, 2021

Choose a reason for hiding this comment

DukeManh Dec 17, 2021

Choose a reason for hiding this comment

DukeManh Dec 17, 2021

Choose a reason for hiding this comment

DukeManh Dec 17, 2021

Choose a reason for hiding this comment

DukeManh commented Dec 17, 2021

HyperTHD commented Dec 18, 2021

humphd commented Dec 18, 2021

sirinoks commented Jan 19, 2022

irenejoeunpark commented Nov 18, 2021 •

edited by gitpod-io bot

Loading

DukeManh left a comment •

edited

Loading