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

Fix 2372: Add commit to Github sidebar #2376

Merged
merged 1 commit into from
Oct 25, 2021
Merged

Fix 2372: Add commit to Github sidebar #2376

merged 1 commit into from
Oct 25, 2021

Conversation

Andrewnt219
Copy link
Contributor

@Andrewnt219 Andrewnt219 commented Oct 21, 2021 •
edited
Loading

Issue This PR Addresses

Resolve #2372

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

Display commits from a post in Github sidebar.

image

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)

@HyperTHD
Copy link
Contributor

It looks good from what I can see, just a quick question.

How are you going to manage an overflow when it comes to posts with multiple commits? In this post for example, there's 6 commits in the post, and it looks like it's going to overflow at any point. We probably want to avoid it taking up too much space if possible.

image

humphd
humphd requested changes Oct 21, 2021
View reviewed changes
src/web/src/components/Posts/Commits.tsx Show resolved Hide resolved
src/web/src/components/Posts/Commits.tsx Outdated Show resolved Hide resolved
@Andrewnt219 Andrewnt219 force-pushed the 2372-github-commit-info branch from dfdae7e to d3b793d Compare October 21, 2021 20:14
@humphd humphd mentioned this pull request Oct 21, 2021
Closed
humphd
humphd requested changes Oct 21, 2021
View reviewed changes
src/web/src/components/Posts/Commits.tsx Outdated Show resolved Hide resolved
@Andrewnt219 Andrewnt219 force-pushed the 2372-github-commit-info branch from a6d4a69 to cc44b1a Compare October 21, 2021 22:42
@Andrewnt219 Andrewnt219 force-pushed the 2372-github-commit-info branch from cc44b1a to b0640b2 Compare October 21, 2021 22:46
@Andrewnt219 Andrewnt219 requested a review from humphd October 21, 2021 22:47
@Andrewnt219 Andrewnt219 marked this pull request as ready for review October 21, 2021 22:48
@Andrewnt219 Andrewnt219 force-pushed the 2372-github-commit-info branch from b0640b2 to 2ee2f05 Compare October 21, 2021 23:41
humphd
humphd requested changes Oct 22, 2021
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.

Really nice. Some small fixes and I think it's good. Do you want to squash as well?

src/web/src/components/Posts/Commits.tsx Outdated Show resolved Hide resolved
@Andrewnt219 Andrewnt219 force-pushed the 2372-github-commit-info branch from 2ee2f05 to 0db87f2 Compare October 22, 2021 17:21
@Andrewnt219 Andrewnt219 force-pushed the 2372-github-commit-info branch from 0db87f2 to 4c2d115 Compare October 22, 2021 17:23
humphd
humphd reviewed Oct 22, 2021
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.

Fantastic!

@Andrewnt219
Copy link
Contributor Author

Andrewnt219 commented Oct 22, 2021 •
edited
Loading

Ah, I lost the squashed commits (description) when I fixed the typo in the last squashed commit.

@Andrewnt219 Andrewnt219 force-pushed the 2372-github-commit-info branch from 4c2d115 to 19d8853 Compare October 22, 2021 17:26
@humphd humphd requested a review from manekenpix October 24, 2021 15:11
@humphd
Copy link
Contributor

humphd commented Oct 24, 2021

@Andrewnt219 can you rebase?

@menghif can you review too please?

humphd
humphd previously approved these changes Oct 24, 2021
View reviewed changes
menghif
menghif reviewed Oct 24, 2021
View reviewed changes
Copy link
Contributor

@menghif menghif left a comment

Choose a reason for hiding this comment

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

This is awesome!
One thing that could be done is reducing the vertical space between commits when there are multiple lines. This is just personal preference though.

Screen Shot 2021-10-24 at 3 09 35 PM

@manekenpix
Copy link
Member

This is awesome! One thing that could be done is reducing the vertical space between commits when there are multiple lines. This is just personal preference though.

I agree, and I'd also check the space between sections (user info, repo, issues, commits), it's a bit inconsistent.

@Andrewnt219 Andrewnt219 force-pushed the 2372-github-commit-info branch from 5ff9b03 to b15670b Compare October 25, 2021 16:28
@Andrewnt219
Copy link
Contributor Author

Andrewnt219 commented Oct 25, 2021 •
edited
Loading

@manekenpix @menghif I just copied data from other posts to demo, so don't mind the duplicates. I also changed to tag <ul> because <p> has Material-UI pre-styled spacing, and <ul> is more accurate semantically.

Flex gap is supported on all modern browsers

image

@Andrewnt219 Andrewnt219 force-pushed the 2372-github-commit-info branch from b15670b to c32a0a9 Compare October 25, 2021 16:31
humphd
humphd previously approved these changes Oct 25, 2021
View reviewed changes
@humphd
Copy link
Contributor

humphd commented Oct 25, 2021

@menghif want to re-review?

@menghif
Copy link
Contributor

menghif commented Oct 25, 2021

@menghif want to re-review?

Looks good to me. Nice change by using <ul>.

@humphd
Copy link
Contributor

humphd commented Oct 25, 2021

@Andrewnt219 squash and rebase this, please, and I'll merge this!

@Andrewnt219
feat: show commits in Github sidebar
5aa659e 
  * add Commits

  * change getCommitNumber()

  * wrap overflow text

  * add space in link's title

  * add SHORT_SHA_LENGTH as constant

  * adjusted space between commits

  * Reset line-height of Material-UI.

  * Change to tag <ul> because <p> has Material-UI pre-style,
  and <ul> is more accurate semantically.
@Andrewnt219 Andrewnt219 force-pushed the 2372-github-commit-info branch from c32a0a9 to 5aa659e Compare October 25, 2021 18:03
@Andrewnt219 Andrewnt219 requested a review from humphd October 25, 2021 18:07
@humphd
Copy link
Contributor

humphd commented Oct 25, 2021

Thanks. Once this passes CI I'll merge.

@humphd humphd merged commit 41841e8 into Seneca-CDOT:master Oct 25, 2021
@Andrewnt219 Andrewnt219 mentioned this pull request Oct 26, 2021
Closed
@JiaHua-Zou JiaHua-Zou mentioned this pull request Feb 2, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@menghif menghif menghif left review comments

@manekenpix manekenpix Awaiting requested review from manekenpix

@humphd humphd Awaiting requested review from humphd

Assignees

@Andrewnt219 Andrewnt219

Labels
area: front-end hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add GitHub commits to post sidebar info
5 participants
@Andrewnt219 @HyperTHD @humphd @manekenpix @menghif