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.

Sign up for GitHub

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

Jump to bottom

Reorganize GithubInfo componment #2806

Merged
merged 1 commit into from
Feb 3, 2022
Merged

Reorganize GithubInfo componment #2806

merged 1 commit into from
Feb 3, 2022

Conversation

JiaHua-Zou
Copy link
Contributor

@JiaHua-Zou JiaHua-Zou commented Feb 2, 2022 •
edited by aserputov
Loading

Issue This PR Addresses

#2380

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

The GitHub components are mixed with other files in the folder .\src\web\src\components\Posts and it's starting to look messy. Move all the GitHub components to the folder .\src\web\src\components\Posts\GitHubInfo to organize it.

Steps to test the PR

  1. pnpm install
  2. pnpm services:start
  3. pnpm start
  4. Go to localhost:8000
  5. Each Post should still have their GitHub components.

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)

@JiaHua-Zou JiaHua-Zou requested a review from HyperTHD February 2, 2022 19:03
@gitpod-io
Copy link

gitpod-io bot commented Feb 2, 2022

@JiaHua-Zou JiaHua-Zou requested a review from aserputov February 2, 2022 19:04
@aserputov aserputov added this to the 2.6 Release milestone Feb 2, 2022
@aserputov aserputov added the type: enhancement New feature or request label Feb 2, 2022
@humphd humphd requested review from Andrewnt219 and menghif February 2, 2022 19:06
humphd
humphd requested changes Feb 2, 2022
View reviewed changes
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.

Rename src/web/src/components/Posts/GitHubInfo/GitHubInfo.tsx to src/web/src/components/Posts/GitHubInfo/index.tsx

DukeManh
DukeManh reviewed Feb 2, 2022
View reviewed changes
src/web/src/components/Posts/GitHubInfo/Post.tsx Outdated
import PostDesktopInfo from './PostInfo';
import PostAvatar from './PostAvatar';
import GithubInfoProvider from '../../GithubInfoProvider';
import { Post } from '../../../interfaces';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Organize imports to components first and interfaces after?

Copy link
Contributor Author

@JiaHua-Zou JiaHua-Zou Feb 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I reorganize the imports. I put all of the GitHub componments together and put the interfaces at the bottom of the list.

@aserputov aserputov linked an issue Feb 2, 2022 that may be closed by this pull request
Reorganize GitHubInfo components into a directory #2380
Closed
@JiaHua-Zou JiaHua-Zou requested review from DukeManh and humphd February 2, 2022 21:17
humphd
humphd requested changes Feb 2, 2022
View reviewed changes
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.

src/web/src/components/Posts/GitHubInfo/Index.tsx is wrong, it should be src/web/src/components/Posts/GitHubInfo/index.tsx, lower case, and then you can import the folder name without any file within it.

DukeManh
DukeManh previously approved these changes Feb 2, 2022
View reviewed changes
@DukeManh DukeManh self-requested a review February 2, 2022 22:11
DukeManh
DukeManh requested changes Feb 2, 2022
View reviewed changes
src/web/src/components/Posts/Timeline.tsx Outdated
@@ -1,7 +1,7 @@
import { ReactElement } from 'react';
import { Container, createStyles } from '@material-ui/core';
import { makeStyles, Theme } from '@material-ui/core/styles';
import PostComponent from './Post';
import PostComponent from './GitHubInfo/Post';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you want to move Post into GitHubInfo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was referring to different PR (#2379, #2367, #2376) and finding what files to move into the GithubInfo file. Should have not include that there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. I move the file back

@JiaHua-Zou
Copy link
Contributor Author

For some reason git did not notice the uppercase change for the file Index.tsx. I had to change the filename to something. commit it, change it to the proper name (index.tsx) then I can finally commit and rebase it.

@DukeManh
Copy link
Contributor

DukeManh commented Feb 2, 2022

@JiaHua-Zou the index file is not a component so don't capitalize the name and use the .tsx extension.

@JiaHua-Zou JiaHua-Zou requested a review from DukeManh February 2, 2022 22:39
DukeManh
DukeManh requested changes Feb 2, 2022
View reviewed changes
src/web/src/components/Posts/Post.tsx Outdated Show resolved Hide resolved
@JiaHua-Zou
Copy link
Contributor Author

Had to push the branch again due to having the wrong import path.

humphd
humphd requested changes Feb 2, 2022
View reviewed changes
src/web/src/components/Posts/Post.tsx Outdated Show resolved Hide resolved
@JiaHua-Zou
Reorganize GithubInfo componment
342216f 
Rename GithubInfo and organize imports

change filename to lowercase

Move Post file back
@JiaHua-Zou JiaHua-Zou requested review from DukeManh and humphd February 3, 2022 00:51
HyperTHD
HyperTHD approved these changes Feb 3, 2022
View reviewed changes
Copy link
Contributor

@HyperTHD HyperTHD left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work

@JiaHua-Zou JiaHua-Zou merged commit 596bf2d into Seneca-CDOT:master Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@humphd humphd humphd approved these changes

@HyperTHD HyperTHD HyperTHD approved these changes

@aserputov aserputov Awaiting requested review from aserputov

@Andrewnt219 Andrewnt219 Awaiting requested review from Andrewnt219

@menghif menghif Awaiting requested review from menghif

@DukeManh DukeManh Awaiting requested review from DukeManh

Assignees

@JiaHua-Zou JiaHua-Zou

Labels
type: enhancement New feature or request
Projects
None yet
Milestone
2.6 Release
Development

Successfully merging this pull request may close these issues.

Reorganize GitHubInfo components into a directory
5 participants
@JiaHua-Zou @DukeManh @humphd @HyperTHD @aserputov