-
Notifications
You must be signed in to change notification settings - Fork 157
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.
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
[#2170] Add Blurbs for Repos #2191
[#2170] Add Blurbs for Repos #2191
Conversation
Co-Authored-By: Ryan Poon <[email protected]>
…implement-blurbs-feature
…kxd/RepoSense into implement-blurbs-feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I like the formatting of the JSON files to make it easier to read. Is it possible to add a negative test case for the BlurbMap
builder?
@sopa301 Let me work on that and update you in the upcoming days! |
@sopa301 I have implemented new test cases for BlurbMap, please do let me know if there are any other aspects that are lacking in tests! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @asdfghjkxd and @sopa301!
Can you please also update the Customizing Reports page in the User Guide to explain how this feature can be used? Maybe it can be merged with the 'Add a title' section to be called 'Personalizing Reports', and the 2 personalization options can be mentioned underneath (titles and blurbs).
One more thing to note is that the current code is implemented such that blurbs can only be used for repos. In case we foresee adding blurbs for author groups (eg. CS3281 summary for a student) or just for repo-author pairs (eg. each CS2103T student mentions their contribution to the tP), then we can create a new issue for this. @damithc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we restyle the blurb box to be more apparent? Right now, the grey color is too light.
Here are my suggestions:
- Use
#F6F8FA
as the fill color - Use
#E9EBEF
as the border color - Use
1px
for the border width - Use
4px
for the border radius - Ensure the top padding is the same as the bottom padding when using a single line of plain text, which is the most common blurb content expected.
Also, it would be great if one of the @reposense/active-developers who did not contribute to this PR added their review too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The frontend and docs LGTM. Once the backend comments are addressed, this should be good to go.
@gok99 @ckcherry23 I have added the new changes as requested! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @sopa301 and @asdfghjkxd for your sprinting efforts today to wrap up this PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just one final request before we can merge this.
@asdfghjkxd @sopa301 This is a really cool feaure and I'm excited to discuss how we can expand on this:
- As discussed earlier, I think having blurbs for author / none grouped views (in addition to repository) would be quite useful. Perhaps one of you could create an issue to discuss this.
- Including blurbs in the one-stop config. I think since the config is meant to provide key features for people interested in quickly showcasing their work, adding the blurbs will add a lot of value. However, having two sources of blurbs will introduce complications. We can discuss this further in One-Stop Config File for Code Portfolio #2161.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the speedy work today
The following links are for previewing this pull request:
|
Thanks @asdfghjkxd for this PR, and @ckcherry23 @gok99 @sopa301 for the reviews/guidance. 💯 |
Noted @ckcherry23 Thanks both @asdfghjkxd and @sopa301 |
Hi prof @damithc , the link should be the link to the branch instead of to the repo (because multiple branches from the same repo can be displayed). For example, https://github.com/AY2324S2-CS2103-F08-2/tp/tree/master instead of https://ay2324s2-cs2103-f08-2.github.io/tp/ If you want to configure the blurbs.md to point to the main/master branch if the repo link is specified, we can open an issue for it. |
@sopa301 Ah, I see. I missed that part. It's working now after I added the branch. Thanks for the clarification.
I guess no need. It's not too much extra work to specify the branch. |
Feature in action https://nus-cs2103-ay2324s2.github.io/tp-dashboard |
Part of #2170
Proposed commit message
Other information
This feature requires Java 11 CI/CD to be implemented.