NEAR-Edu / near-certification-tools

2 stars 2 forks source link

Feature/certificate designs #5

Closed norrec99 closed 2 years ago

norrec99 commented 2 years ago

BEFORE

image

AFTER

image

ryancwalsh commented 2 years ago

We're removing that field (competencies/description) anyway. Dan is updating the designs to remove it, right?

On Fri, Feb 11, 2022 at 8:44 AM Cagatay Soyer @.***> wrote:

@.**** commented on this pull request.

In web-app/helpers/certificate-designs.ts https://github.com/NEAR-Edu/near-certification-tools/pull/5#discussion_r804662650 :

  • const fillStyle = #${Math.floor(Math.random() * 16777215).toString(16)}; // This is just temporary and will be removed once we have final designs for each program's certificate. https://css-tricks.com/snippets/javascript/random-hex-color/
  • const font = 40px '${fontFamily}' bold;
  • const newDate = date.split(' ')[0];
  • const newExpiration = expiration.split(' ')[0];
  • const programDescription1 = programDescription.split(',')[0];
  • const programDescription2 = programDescription.split(',')[1];

I tried to explain why i did such a thing in here: https://discord.com/channels/828768337978195969/906115083250307103/941290874573320203

But basically, if I don't split the description, it looks like this [image: image] https://user-images.githubusercontent.com/45902312/153601833-ce07d249-c976-4e94-8d38-8fdaeb0b7f5b.png

After splitting it, [image: image] https://user-images.githubusercontent.com/45902312/153601926-8d3243f5-33b8-4638-9ea7-d0369bb9a305.png

Maybe it is not a best practice but idk how to fix it otherwise. So for now, I can not change it.

— Reply to this email directly, view it on GitHub https://github.com/NEAR-Edu/near-certification-tools/pull/5#discussion_r804662650, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAP5MXLBAYIJF3SZDEDHJJTU2UHFHANCNFSM5OARVQFA . You are receiving this because you commented.Message ID: @.***>

norrec99 commented 2 years ago

I never heard that Dan is working on removing the "description" from the design anywhere. Where did you see it? Maybe I missed something. So does that mean, the certificates won't have descriptions on them?

norrec99 commented 2 years ago

fb76781 e94162b For the latest commits, here are the changes:

I don't have the latest svgs yet so this is just a placeholder. As soon as I get all the svgs from Dan, It will be ready for production. Before Untitled

After image

So, I deleted the populateAnalystCert function because we are now using switch to generate different certs.

I also deleted the switch in the web-app/pages/api/cert/[imageFileName].ts for the same reason above.

ryancwalsh commented 2 years ago

The Discord near-certification-tools channel.

Sherif said he doesn't want competencies. I'm pretty sure he means this field.

On Fri, Feb 11, 2022, 9:02 AM Cagatay Soyer @.***> wrote:

I never heard that Dan is working on removing the "description" from the design anywhere. Where did you see it? Maybe I missed something.

— Reply to this email directly, view it on GitHub https://github.com/NEAR-Edu/near-certification-tools/pull/5#issuecomment-1036243714, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAP5MXKBUBZOR2URG67IDR3U2UJF7ANCNFSM5OARVQFA . You are receiving this because you commented.Message ID: @.***>

norrec99 commented 2 years ago

With the last commits, the page looks like this

image

ryancwalsh commented 2 years ago

@norrec99 Thanks for that helpful screenshot. It seems that the certificates have a blue border, so I wonder if we should remove the light gray border surrounding each of those, since it seems potentially unnecessary.

Currently, http://localhost:3000/account/hatchet.testnet throws this error for me:

error - (api)/helpers/certificate-designs.ts (38:50) @ populateDeveloperCert
TypeError: Cannot read properties of undefined (reading 'split')
  36 |   const programDescription4 = programDescription.split('program ')[1];
  37 |   const programDescription1 = programDescription.split('by')[0];
> 38 |   const programDescription2 = programDescription4.split('on ')[0];
     |                                                  ^
  39 |   const programDescription3 = programDescription4.split('contracts ')[1];
  40 | 
  41 |   const gray = '#757575';

and looks like this: image

So, this helps show that you'll need to handle that programDescription section differently. You'll need to figure out a way other than splitting it based on hard-coded text values that you were searching for, since you can't guarantee that those pieces of text will exist.

I found https://stackoverflow.com/questions/4991171/auto-line-wrapping-in-svg-text

But embedding HTML doesn't seem like a good idea since we want these images to be shareable as PNGs too (on Twitter, etc).

So, you can explore whether you can come up with a solution. If you feel blocked, then maybe you should explain to Dan, Sherif, me, and others in the Discord channel and recommend temporarily removing these multi-line fields. And then if we can ever figure out a graceful way of adding a multi-line description to an SVG, we can add it later.

What do you think?

norrec99 commented 2 years ago

Currently, http://localhost:3000/account/hatchet.testnet throws this error for me:

error - (api)/helpers/certificate-designs.ts (38:50) @ populateDeveloperCert
TypeError: Cannot read properties of undefined (reading 'split')
  36 |   const programDescription4 = programDescription.split('program ')[1];
  37 |   const programDescription1 = programDescription.split('by')[0];
> 38 |   const programDescription2 = programDescription4.split('on ')[0];
     |                                                  ^
  39 |   const programDescription3 = programDescription4.split('contracts ')[1];
  40 | 
  41 |   const gray = '#757575';

You are having this error because I've changed the "description" text according to what Sherif said here https://discord.com/channels/828768337978195969/906115083250307103/942726014243581982

Because I'm not handling this "description" in a graceful way, everybody gets an error if the text doesn't match exactly the same as Sherif said.

I found https://stackoverflow.com/questions/4991171/auto-line-wrapping-in-svg-text

I'll take a look at this one tomorrow! Thanks for that.

But embedding HTML doesn't seem like a good idea since we want these images to be shareable as PNGs too (on Twitter, etc).

Sorry, I really don't aware of the difference until you said it so I'll take a look into that, too

Also, I added this "CERTIFICATE OF ACHIEVEMENT" as the last commit. We need to think about this whether we want to keep it or not

ryancwalsh commented 2 years ago

@norrec99 Yeah, we don't want our code to be brittle and break / throw an error just because data changes underneath. I.e. we want the pages / images to look reasonable regardless of what the on-chain data says, ideally.

When I wrote "But embedding HTML doesn't seem like a good idea since we want these images to be shareable as PNGs too (on Twitter, etc)." I was referring to the workarounds suggested in the StackOverflow answers. They suggested using embedded HTML as a way to show multi-line text. That might be fine if we only ever wanted to display the SVGs on our webpage. But in reality, we want the SVGs to be exportable and also downloadable as PNGs. If we were just displaying a webpage, we would just use HTML and CSS from the beginning and never bother with SVG at all. So that workaround suggestion in those S.O. answers is kind of bizarre.

norrec99 commented 2 years ago

@ryancwalsh I'm looking for a way to solve the programDescription but so far I couldn't find a way to do it.

I found this https://stackoverflow.com/questions/16701522/how-to-linebreak-an-svg-text-within-javascript

They are talking about adding something like this;

<tspan x="0" dy="1.2em">very long text</tspan>
<tspan x="0" dy="1.2em">I would like to linebreak</tspan>

But even in this case, I guess we need to use .split() and put those into the <tspan> which is not the behavior we want.

I'll research a bit more and if I can't find anything useful, I will ask for help on the near-certification-tools channel.

norrec99 commented 2 years ago

The other issue I want to mention is the alignment of the elements

Here you can see the program name is close to the right-hand side because the text is too long (relatively) image

On the other hand, this is close to the left-hand side because the text is too short (relatively) image

This issue also applies to the accountName section and probably to the instructor too. If those texts will be too long or too short then the alignment will be broken because we are applying the same width and height to all.

Is there a way to prevent such behavior?

ryancwalsh commented 2 years ago

@norrec99 See https://duckduckgo.com/?q=canvas+how+center+align+text&t=brave&ia=web

Also I'm curious whether https://stackoverflow.com/a/28553412/470749 helps the multiline problem.

norrec99 commented 2 years ago

I think we find a way to this multiline problem. Thanks to @rashaabdulrazzak we come up with a function that splits long text into shorter lines.

There are a couple of questions and comments on the latest commit as I wrote them as comments but the major problem is this image There is an extra space that I couldn't remove at the beginning of the text.

Also, font-weight doesn't really work at all! That's another issue.

ryancwalsh commented 2 years ago

Did you search for "how to remove a space from the beginning of a string"?

Also, if /fonts/Manrope-VariableFont_wght.ttf doesn't support ExtraBold, and if you need ExtraBold, then it sounds like you'll need an additional font file for that weight.

Also, since Dan chose not to use a monospaced font for the dates in the bottom right, and since they aren't right-aligned, they end up looking wonky. I think those should be right-aligned. And maybe that will also mean that we should remove the labels "Issued:" and "Expiring:" from the background image and dynamically insert them into the right-aligned text.

The fonts for the instructor (label and value) also look weird currently.

You're making progress, though!

norrec99 commented 2 years ago

I just fixed the leading space by using trim()

Also, if /fonts/Manrope-VariableFont_wght.ttf doesn't support ExtraBold, and if you need ExtraBold, then it sounds like you'll need an additional font file for that weight.

I don't think it is the reason because "medium", "regular" or "bold" are not working either. Even though I put Manrope-Bold.tff, doesn't seem to be working.

I tried to change the order of this. const accountFont = `60px '${fontFamily}' bold`; For example like that const accountFont = `bold 60px '${fontFamily}'`; Still, nothing changed

And maybe that will also mean that we should remove the labels "Issued:" and "Expiring:" from the background image and dynamically insert them into the right-aligned text.

If we are going to do this, we should also do it for the instructor I guess because it looks weird as you said.

You're making progress, though!

Thanks!

ryancwalsh commented 2 years ago

The instructor field won't have an alignment problem, though. So the problem there is just that we should adjust the font style and/or size of either the label or the value.

@rashaabdulrazzak Do you think you could help figure out how to get the fonts to look good? For example, figure out why bold isn't working. Thanks.

norrec99 commented 2 years ago

@ryancwalsh I fixed the font-weight problem.

image

The instructor field won't have an alignment problem, though. So the problem there is just that we should adjust the font style and/or size of either the label or the value.

Do you still think the "instructor" field looks weird?

cc @rashaabdulrazzak

norrec99 commented 2 years ago

Also, since Dan chose not to use a monospaced font for the dates in the bottom right, and since they aren't right-aligned, they end up looking wonky. I think those should be right-aligned. And maybe that will also mean that we should remove the labels "Issued:" and "Expiring:" from the background image and dynamically insert them into the right-aligned text.

@ryancwalsh So, I was experimenting with the date field and realized something minor. For example, think about this two date "2021-11-28" "2022-08-16" I put the dates exactly on the same x-axis above on purpose and they look like they have the same width. But this scenario won't work if the font Github uses here would be the "manrope" that we use in our project. It's because "1" has a smaller width than "2" or "0" has bigger width than "1" in the "manrope". That's why we see an alignment problem in the date fields on the cert. Even though we make them right-aligned, we have a high chance to encounter an alignment problem.

See the differences below; image

image

wdyt?


P.S.

After I talked with Dan, he suggested using another font for accountName, date, programName, instructor and tokenId now It looks like this image

norrec99 commented 2 years ago

What do you think about these designs? @ryancwalsh @rashaabdulrazzak @ozanisgor

With many certs image With one cert image

With many certs image With one cert image


This is the current one With many certs image With one cert image

ozanisgor commented 2 years ago

What do you think about these designs? @ryancwalsh @rashaabdulrazzak @ozanisgor

I like the first one but If person will have only one certificate It will not look good when aligned to left. If there is a way to make cert aligned to center when there is only one then my choice is the first alternative that you share but otherwise current version is also looking good.

norrec99 commented 2 years ago

@ryancwalsh I was working on this problem https://github.com/NEAR-Edu/near-certification-tools/pull/5#issuecomment-1040127826

To fix this issue, I changed the structure a little. If you remember when I asked, "do we need to create a function for each cert to generate an image?" and you said, "we can use a switch, if all the properties are the same for each cert". Then we decided to go with the "switch solution". So I literally create a function for each cert in order to play them separately. Now we can change the position of each element on the cert.

Here is the look of the page image

I haven't committed it yet. What do you think about this solution?

ryancwalsh commented 2 years ago

@norrec99 Thanks for the screenshots and all of these comments. I don't know what you're saying in the most recent comment, though, because you didn't push any commits. If you're nervous about messing with your current branch, you could always just branch off of that branch and push commits there for me to see. Then we can abandon and delete whichever branch we don't like.

The new font family that you got from Dan is what I was talking about when I said "monospace". Monospace fonts take the same width for each character, unlike most fonts. It makes design alignment much easier but is less pretty. I think we can proceed with monospace in the short run. Later, we can review the other suggestion I was making since I don't think you understood, and we can see what people prefer.

@ozanisgor and @rashaabdulrazzak you should hear this too: pushing commits to GitHub is not the same thing as then also submitting a PR on GitHub. We should only submit a PR when we think something is ready to be reviewed and deployed. We've all been a little sloppy about this recently. And we've left these PRs open rather than changing them into "draft" mode or canceling them.

E.g. you know that I wouldn't approve this PR currently because this is the result I see:

image

In general, whenever possible, it's best to work iteratively. It's best for the reviewer (and the rest of the team too) if you make solid progress on tiny pieces and make those pieces production-quality, and then submit a PR for that, and that makes it easy for me to review it, approve it, and merge it into develop for everyone else.

This means that then we'll have fewer conflicts and also our server will show improvements more frequently.

ryancwalsh commented 2 years ago

@eadsoy @hiba-machfej It's worth subscribing to all the conversations of this repo if you haven't already so that you can follow along some tips I'm sharing.

ryancwalsh commented 2 years ago

For example, @norrec99 when I saw the screenshot in the PS part of your comment at https://github.com/NEAR-Edu/near-certification-tools/pull/5#issuecomment-1041239687 I thought "That looks like a good enough result for right now that it would be worth deploying". But the current state of the commits pushed to this branch (i.e. this PR) aren't what's shown in that screenshot. So I can't approve and deploy it.

My general approach is that I always lean towards approving (if something is above a minimum threshold of quality) even if I have ideas for improving it further later in a next iteration.

norrec99 commented 2 years ago

E.g. you know that I wouldn't approve this PR currently because this is the result I see:

I don't understand why are you seeing something like that. You should have seen this image with the latest commit image

Maybe you should use my dev-account: dev-1644856769338-22652649644211 idk

I'll try to replicate the error you got on this post https://github.com/NEAR-Edu/near-certification-tools/pull/5#issuecomment-1041521643

ryancwalsh commented 2 years ago

@norrec99 I think maybe the problem I'm experiencing locally is related to the fact that you have 2 files that are basically the same file name but with different capitalization. web-app/public/certificate-backgrounds/NCD_certificate.svg

Different systems treat capitalization differently. My Mac seems not to like it. I don't know the solution yet. But I think you'll probably need to remove one from the repo.

By the way, I know I'm giving you a lot more work to do in these suggestions of mine. Maybe @rashaabdulrazzak or @ozanisgor can work with you if they have capacity and have finished their stuff already.

norrec99 commented 2 years ago

This is awkward because I deleted the file with lower letters a long time ago. In this commit to be specific: https://github.com/NEAR-Edu/near-certification-tools/pull/5/commits/a2b8874786f5e27ce708a0dd0f90b817f685b519 Now I only have these files: image

By the way, I know I'm giving you a lot more work to do in these suggestions of mine. Maybe @rashaabdulrazzak or @ozanisgor can work with you if they have capacity and have finished their stuff already.

We are already working together, I ask lots of questions to them and we make calls to discuss those questions. But please put something more on my shoulders, this would only make me happier :)

ryancwalsh commented 2 years ago

@norrec99 You can see that the lowercase file still exists at https://github.com/NEAR-Edu/near-certification-tools/tree/feature/certificate-designs/web-app/public/certificate-backgrounds

I will try to delete it.

ryancwalsh commented 2 years ago

We're making progress! Here is what main (production server) looks like now: https://near-certification-tools-tpq1.onrender.com/account/hatchet.testnet

@norrec99 I think the alignment of this part is messed up:

image
norrec99 commented 2 years ago

Yes, I'm aware of this issue. Actually, I was trying to explain this alignment issue in this post https://github.com/NEAR-Edu/near-certification-tools/pull/5#issuecomment-1041431054

The image you see on that post is the one that solves the alignment problem by separating each cert with a different function.

This is the one I didn't commit. I will commit this change and all the other feedback you mentioned tomorrow.

ryancwalsh commented 2 years ago

Ok, but I don't understand why it would be anything specific to each program. Instead, we just want all program names centered. I would have thought that would be simple but I guess I'm misunderstanding. 😃

On Wed, Feb 16, 2022, 10:42 AM Cagatay Soyer @.***> wrote:

Yes, I'm aware of this issue. Actually, I was trying to explain this alignment issue in this post #5 (comment) https://github.com/NEAR-Edu/near-certification-tools/pull/5#issuecomment-1041431054

The image you see on that post is the one that solves the alignment problem by separating each cert with a different function.

This is the one I didn't commit. I will commit this change and all the other feedback you mentioned tomorrow.

— Reply to this email directly, view it on GitHub https://github.com/NEAR-Edu/near-certification-tools/pull/5#issuecomment-1041923153, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAP5MXMQB4OPWYKIZGKSFLTU3POX3ANCNFSM5OARVQFA . You are receiving this because you modified the open/close state.Message ID: @.***>

norrec99 commented 2 years ago

I think it is because of this function

'addText(canvas, programName, programFont, black, 240, 680);'

We are saying that no matter what every programName starts in the 240px at the x-axis. But some programNames are longner and some are shorter. This causes the alignment problem.

Maybe there is a better way than hard-coding the x-axis but I decided to do a different function for each cert so that I can change

'addText(canvas, programName, programFont, black, 240, 680);'

this function for each cert.

ryancwalsh commented 2 years ago

@norrec99 Thanks for that explanation. The better approach here is not to hard-code X position values based on what we assume the program names will be. Instead we want the X pos value that we pass to addText to be talking about the left position of the "div" (or equivalent). Then we also want (maybe via a new argument) to be able to do the equivalent of text-align center in certain cases, such as for the program name field.

https://github.com/NEAR-Edu/near-certification-tools/blob/develop/web-app/helpers/certificate-designs.ts#L22

norrec99 commented 2 years ago

@ryancwalsh What do you think of this solution for the alignment problem? e5924646c58b536e4854b79eac4667b14198136e

Changed text alignment from "left" to "center" except for the programDescription and instructor Made some calculations for the x-axis in the addText() function. It might be confusing but it works for now 😅

Page looks like this: image

It also work for longer names image

cc @ozanisgor @rashaabdulrazzak