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.

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

Nus templates new #125

Open
wants to merge 44 commits into
base: master
Choose a base branch
from
Open

Nus templates new #125

wants to merge 44 commits into from

Conversation

ccewuz
Copy link
Contributor

@ccewuz ccewuz commented Jan 14, 2022


name: Certificate Template Addition
about: This is the workflow for requesting new certificate templates to be added to
the OpenCerts repository
title: "[New Template]"
labels: new template
assignees: ''


Pull Request Guidelines for Adding Certificate Templates

This document is a work in progress but here are some basic checks. As these are only basic guidelines, meeting the below doesn't indicate there will be no issues with your pull request.

Pre-merge checks

  • Did not modify any files outside of your organisation's template folder (e.g package-lock.json or anything else)
  • Ensure that your code has been rebased on top of latest OpenCerts master
  • Linter issues resolved (Run npm run lint:fix to see issues)
  • npm run test passes
  • npm run test:integration passes
  • Travis Build passes

Certificate Template

  • No more than 5 templates or 25 added/modified files in the pull request
  • Ensure that your .opencert file's data complies with the intentions of the OpenCerts' schema - e.g recipient related information is inside the recipient object, etc.
  • Integration test for each template that checks that the correct rendering is done given a sample certificate
  • Sample certificate file included for each template, located alongside the integration test for each template
  • Sample certificates must obviously be a sample certificate
    • Obviously fictitious name
    • Obviously sample signatory images
  • No fixed-size raster images as part of certificate layout
  • Mobile responsive design
  • Date parsing should be localised to template author's timezone
  • Webpack chunking code is correct
    • Has chunking code
    • Same chunking code as the other certificates belonging to that institute
  • Certificate Store Addresses have been updated
    • Template Whitelist
    • Registry
  • Template should not be using resources(images etc.) on the website outside of their own folder (e.g institute logo shouldn't be used from /static because there's no guarantee it will not change)

ccewuz and others added 30 commits October 4, 2019 17:23
Copy link
Contributor Author

@ccewuz ccewuz left a comment

Choose a reason for hiding this comment

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

change is as desired. no impact to logic

@lgtm-com
Copy link

lgtm-com bot commented Jan 14, 2022

This pull request introduces 2 alerts when merging 4aeb1db into cb5936f - view on LGTM.com

new alerts:

  • 2 for Assignment to constant

deleted extra const declaration
Copy link
Contributor Author

@ccewuz ccewuz left a comment

Choose a reason for hiding this comment

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

done the fix

@ccewuz
Copy link
Contributor Author

ccewuz commented Jan 17, 2022

please help with merge. thanks

@Nebulis
Copy link
Contributor

Nebulis commented Jan 19, 2022

Hi, I'm sorry but this repository is not meant to accept new templates since the 1st Jan

Please reach out to ssg

@ccewuz
Copy link
Contributor Author

ccewuz commented Jan 20, 2022

This pull consists updates to existing templates as well. If cancel this and resubmit a subset of changes w/o new templates, will it be accepted?

@Nebulis
Copy link
Contributor

Nebulis commented Jan 21, 2022

Yes we can approve changes to exisiting templates

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants