unburn / musicard

Musicard is a futuristic music card canvas library
GNU General Public License v3.0
81 stars 22 forks source link

Enhancements and Code Fixes: Constructor Parameters, JSDoc, Time Validation, Color Support, and Style Compliance #12

Closed devaoto closed 1 year ago

devaoto commented 1 year ago

In this pull request, I've added some features and fixed some issues.

Added:

Fixed:

Elitex07 commented 1 year ago

@codeblitz97 After checking the code, am not able to find anything, which should be merged into the repository. Can you define any?

devaoto commented 1 year ago

@Elitex07 I've added JSDoc for VSCode intelligence (https://jsdoc.app/) it's just comments but it provides intelligence to the VSCode and maybe for other code editors too, then I saw that the class name actually doesn't follow the rules of naming class, so I've changed "musicCard" to "MusicCard" then for making things easier, I've added an object in the main class which has all of the options. For utility, I've added "random" options in the colorFetch() method so we can use: color: "random" in the class object or the class method. Also created an interface for typescript. Last, for error handling I've created a new method that will check if the provided "endtime" or "starttime" is in the format: "min:seconds (0:00 or whatever)" or not, it will throw an error if the time is not in the valid format

Elitex07 commented 1 year ago

@codeblitz97 am asking again. Tell things which should be merged.

devaoto commented 1 year ago

@Elitex07 https://github.com/a3pire/musicard/pull/12#issuecomment-1737694580

Elitex07 commented 1 year ago
Elitex07 commented 1 year ago

After this, if you have anything which should be merged, like the error handling of time formats. You should create another pull request with only those changes.

devaoto commented 1 year ago

So you're saying that I should revert the code to the old version and just add the time format checking and nothing else?

Elitex07 commented 1 year ago

Kind of yea.

devaoto commented 1 year ago

Okay then, removing everything except time checking, and those spaces were added by my formatter (prettier) automatically so I had no control over that

Elitex07 commented 1 year ago

np

devaoto commented 1 year ago

Updated

flameface commented 1 year ago

@codeblitz97 I appreciate your work, and I want to make things clear.

Before you, one guy already sent that class thing to be direct in musicard({ here }), which I just denied because we can't change it as it's our way of presenting it, and we can't change it directly now.

You are saying somewhat about the rules of classes. It's musicCard or MusicCard doesn't matter; I checked the source, and they want to say it shouldn't be music_Card

Last, the time formatting; it's a canvas card; you can do whatever you want; there are no restrictions; and it's not required for now, i think.

devaoto commented 1 year ago

@flameface Yeah, I understand but for the time formatting, will it still work if I provide something like: "oneminute:fiveseconds" or whatever? I don't think so to prevent that I've added that

flameface commented 1 year ago

I like the time formatting, I will check and let you tomorrow. thank you

flameface commented 1 year ago

@flameface Yeah, I understand but for the time formatting, will it still work if I provide something like: "oneminute:fiveseconds" or whatever? I don't think so prevent that I've added that

It will be okay, but till now, I didn't see anyone do that.

Elitex07 commented 1 year ago

fr 💀, my bad. Kindly revert it

devaoto commented 1 year ago

@Elitex07 Added package.json and I'm closing this pull but I'm working on something else

flameface commented 1 year ago

All is good to go.

flameface commented 1 year ago

will you pull the request again of that time formatting

devaoto commented 1 year ago

Sure