-
Notifications
You must be signed in to change notification settings - Fork 132
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
MarkBind Template for Student Portfolio #2398
MarkBind Template for Student Portfolio #2398
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2398 +/- ##
==========================================
+ Coverage 51.02% 51.11% +0.08%
==========================================
Files 124 124
Lines 5372 5355 -17
Branches 1159 1152 -7
==========================================
- Hits 2741 2737 -4
+ Misses 2341 2328 -13
Partials 290 290 ☔ View full report in Codecov by Sentry. |
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 the work @jingting1412! The portfolio page looks nice.
Hi @jingting1412, actually if you are looking for inspiration, you can check out these portfolios that the team thinks is nice! |
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.
Hi @jingting1412 thanks for the changes!
One comment on moving the icons and thumbnails
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 work @jingting1412
Sorry @jingting1412, one more thing! |
Yep added! Sorry for not noticing earlier |
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! Will wait for more comments before merging :)
@kaixin-hc @EltonGohJH @itsyme
Hi, thanks for the PR @jingting1412! I've taken a look at the template, and I just have a few broad comments / suggestions that has to do with the UI / UX rather than the implementation itself:
These are suggestions rather than directions, and feel free to counter these if you feel that they are not preferable 👍 |
Thanks everyone for the review! I've updated the site and it looks like this now: https://portfolio-template-markbind.netlify.app/ |
{{ vue }} | ||
</div> | ||
<div class="card-body"> | ||
<a href="#" class="btn btn-primary">View on Github</a> |
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.
Suggest that this link should go to MarkBind github and the one below should go to MarkBind website
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.
Hi thanks for the suggestion, I've added it and you can check it out here: https://portfolio-template-markbind.netlify.app/
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.
Great! @jingting1412 the button next to this seems wrong tho? What is "post"
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.
I was thinking that "Devpost" seems a bit inaccurate, since we're linking to documentation, but yeah "post" might be a little confusing too 😓. I'll change it back to Devpost
…o/expected/index.html Co-authored-by: Kevin Eyo <[email protected]>
…o/expected/index.html Co-authored-by: Kevin Eyo <[email protected]>
…o/expected/index.html Co-authored-by: Kevin Eyo <[email protected]>
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 except for the broken link
</a> | ||
</span> | ||
</div> | ||
<a class="btn btn-primary" href="./contents/assets/ug_template.docx" download="UG-template-resume" role="button">Resume</a> |
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.
Perhaps using Download Resume could be more indicative to the viewer that it is a button to download as my expected intention when clicking the button is to be brought to a page showing the resume. Another way to go about this could be to exporting the docx file to pdf to allow it to be opened in a new tab instead of forcing a download. Imo using the pdf would be my preference
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.
Yep makes sense! I've changed it to the pdf file, you can check it again here: https://portfolio-template-markbind.netlify.app/
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 research and effort put into creating this template!
cb84513
to
69ec838
Compare
@yucheng11122017 Each PR must have a SEMVER impact label, please remember to label the PR properly. |
What is the purpose of this pull request?
Overview of changes:
Added a template portfolio website for users based on the existing default template generated when running
markbind init ---template
and existing portfolio themes available on jekyll.#2385
Anything you'd like to highlight/discuss:
Testing instructions:
run
markbind init --template portfolio
and see if contents are loadedProposed commit message: (wrap lines at 72 characters)
Add portfolio template for init command
Checklist: ☑️