Skip to content

Conversation

IanCheah
Copy link
Contributor

@IanCheah IanCheah commented Feb 5, 2025

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Fixes #2418

Overview of changes:

  • Deployed portfolio site on netlify
  • Updated the templates.md page

Anything you'd like to highlight/discuss:
The preview of the portfolio site can be seen here

Testing instructions:

  1. run the command markbind s
  2. navigate to the templates page
    i. Click on the USER GUIDE tab located at the top of the home page
    ii. Click on the Templates tab located at the side, under Working with Sites

Proposed commit message: (wrap lines at 72 characters)
Deploy portfolio on Netlify


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

@lhw-1
Copy link
Contributor

lhw-1 commented Feb 7, 2025

The porfolio site is publicly accessible using the following url https://markbind-template-portfolio.netlify.app/.

Quick link here

@lhw-1
Copy link
Contributor

lhw-1 commented Feb 7, 2025

@IanCheah The deployment itself looks rather different from this one here mentioned in the PR #2398 for the template; there are of course bound to be some changes, but the icons etc. seem to be displayed incorrectly?

@gerteck
Copy link
Contributor

gerteck commented Feb 7, 2025

Reference Documentation Site in Question: https://markbind.org/userGuide/templates.html

Something doesn't seem right... The buttons represent deploy to Netlify links, not sample sites of the templates.

image

These are actually quick deploy links provided by MarkBind, that create a repository for you and deploy in a single click.

In fact, this highlights that the project template netlify-deploy button is also wrongly labelled.
It was probably overlooked in #2400 because a great deal of effort was spent in the design of the site, such that the documentation was wrongly labelled.

Also the site https://markbind-template-portfolio.netlify.app/ looks broken on my end too. (Needs to be investigated).

I have a few suggestions:

  • Add maybe a separate column as 'sample site' or 'example site' for each template.

  • Consider adding this for default and minimal as well.

  • Fix the project template quick deploy link, because it's not a quick deploy link. Instead, maybe put it under sample site or related, as it's quite helpful.

  • Fix and simplify the portfolio template, right now it seems a bit convoluted (and also seems broken). As discussed in our last meeting, it could be beneficial to simplify and even target certain audiences (i. e. cs2103t / cs2113 students) since they have a touchpoint with markbind and are probably the most likely to be new users. This could mean adding a sample tP project, and maybe an orbital section as well.

These suggestions could be made out into separate issues as well to be appropriate handled and tracked.

Cheers 👍

@Tim-Siu
Copy link
Contributor

Tim-Siu commented Feb 10, 2025

Reference Documentation Site in Question: https://markbind.org/userGuide/templates.html

Something doesn't seem right... The buttons represent deploy to Netlify links, not sample sites of the templates.

image

These are actually quick deploy links provided by MarkBind, that create a repository for you and deploy in a single click.

In fact, this highlights that the project template netlify-deploy button is also wrongly labelled. It was probably overlooked in #2400 because a great deal of effort was spent in the design of the site, such that the documentation was wrongly labelled.

Also the site https://markbind-template-portfolio.netlify.app/ looks broken on my end too. (Needs to be investigated).

I have a few suggestions:

  • Add maybe a separate column as 'sample site' or 'example site' for each template.
  • Consider adding this for default and minimal as well.
  • Fix the project template quick deploy link, because it's not a quick deploy link. Instead, maybe put it under sample site or related, as it's quite helpful.
  • Fix and simplify the portfolio template, right now it seems a bit convoluted (and also seems broken). As discussed in our last meeting, it could be beneficial to simplify and even target certain audiences (i. e. cs2103t / cs2113 students) since they have a touchpoint with markbind and are probably the most likely to be new users. This could mean adding a sample tP project, and maybe an orbital section as well.

These suggestions could be made out into separate issues as well to be appropriate handled and tracked.

Cheers 👍

Great catch on the misleading "Deploy to Netifly" label. I also agree that we should add a seperate column about sample deployment.👍

Copy link

codecov bot commented Feb 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.86%. Comparing base (92d2373) to head (a667c5f).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2593   +/-   ##
=======================================
  Coverage   51.86%   51.86%           
=======================================
  Files         127      127           
  Lines        5474     5474           
  Branches     1201     1201           
=======================================
  Hits         2839     2839           
  Misses       2340     2340           
  Partials      295      295           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lhw-1
Copy link
Contributor

lhw-1 commented Feb 20, 2025

Hi, what's the progress on this PR?

@IanCheah
Copy link
Contributor Author

Hi @lhw-1, as I am working on issue #2594 I have not made the changes to the PR. will incorporate the changes discussed by the end of this week

@IanCheah IanCheah marked this pull request as draft February 23, 2025 17:33
@IanCheah IanCheah marked this pull request as ready for review March 9, 2025 12:06
Copy link
Contributor

@gerteck gerteck left a comment

Choose a reason for hiding this comment

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

This looks awesome!

The updated page can be viewed here:
https://deploy-preview-2593--markbind-master.netlify.app/userguide/templates#templates

Old page for reference.

LGTM

@gerteck gerteck requested a review from lhw-1 March 9, 2025 13:13
@lhw-1 lhw-1 added the r.Minor Version resolver: increment by 0.1.0 label Mar 10, 2025
@lhw-1
Copy link
Contributor

lhw-1 commented Mar 10, 2025

Personally, the PR is LGTM - with just one nit. Instead of using the icon for the sample sites, it might be better to just use text-based buttons to be consistent with the rest of the user guide.

Are there places where we are using icon buttons for similar purpose @IanCheah?

@IanCheah
Copy link
Contributor Author

I agree with that, I wanted to use a button because the deploy on netlify also uses a button. I overlooked the fact that it uses a text-based one.

Personally, the PR is LGTM - with just one nit. Instead of using the icon for the sample sites, it might be better to just use text-based buttons to be consistent with the rest of the user guide.

Are there places where we are using icon buttons for similar purpose @IanCheah?

@lhw-1
Copy link
Contributor

lhw-1 commented Mar 12, 2025

I agree with that, I wanted to use a button because the deploy on netlify also uses a button. I overlooked the fact that it uses a text-based one.

@IanCheah Will you be updating the PR with this soon? If so, please do update it - if not, we may merge this PR in first.

@IanCheah
Copy link
Contributor Author

I agree with that, I wanted to use a button because the deploy on netlify also uses a button. I overlooked the fact that it uses a text-based one.

@IanCheah Will you be updating the PR with this soon? If so, please do update it - if not, we may merge this PR in first.

Yes, I am done with it. What do you think about this?
image

Copy link
Contributor

@lhw-1 lhw-1 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @IanCheah for the PR :)

@lhw-1 lhw-1 merged commit acf4f7a into MarkBind:master Mar 12, 2025
8 checks passed
Copy link

@lhw-1 Each PR must have a SEMVER impact label, please remember to label the PR properly.

Copy link

Welcome, @IanCheah! 🎉 Thank you for your contribution to the MarkBind project!

@lhw-1, please remember to add @IanCheah as an official contributor to our repository.

See the full list of contributors here. ✨

@lhw-1 lhw-1 added this to the v5.6.0 milestone Mar 13, 2025
@lhw-1
Copy link
Contributor

lhw-1 commented Mar 14, 2025

@all-contributors please add @IanCheah for doc

Copy link
Contributor

@lhw-1

I've put up a pull request to add @IanCheah! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
r.Minor Version resolver: increment by 0.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set up deployment for student portfolio template
4 participants