search

Catalyst-IN / OpenSourceEvents-Frontend

This website contains a list of open source events and hackathon.
https://opensourcefrontend.netlify.app/
MIT License
64 stars 74 forks source link

Add Steve Jobs quotes #155

Closed anubhavpulkit closed 3 years ago

anubhavpulkit commented 3 years ago

Description

We have some vaccant space at bottom of home page, We can fill by adding famous Steve jobs quotes.

Mocks

issue1

Acceptance Criteria

Update [Required]

Definition of Done

nilisha-jais commented 3 years ago

I want to work on it

anubhavpulkit commented 3 years ago

I want to work on it

Send a clean PR ASAP.

Neox63 commented 3 years ago

I'm on it, you'll get the PR in 1h max.

Neox63 commented 3 years ago

Done, let me know if you want me to change anything about it.

nilisha-jais commented 3 years ago

I want to work on it

Send a clean PR ASAP.

Should I work or is it done. I planned to add a slideshow of quotes

anubhavpulkit commented 3 years ago

I want to work on it

Send a clean PR ASAP.

Should I work or is it done. I planned to add a slideshow of quotes

Send your PR with changes I mentioned in @Neox63 PR.

Neox63 commented 3 years ago

Hey, thank you for merging. But you merged the 2 PR's and there's huge incoherences in the code rn. There is my script in the js folder (quote.js) that references a deleted tag in index.html (because of the merge of the 2nd PR) AND the script tag of @nilisha-jais that references the 'spam' id at line 845.

It's very unlogical and it make the code heavier for nothing. i read the PR of @nilisha-jais and tbh, the script should not work like that.

I saw severals things in the @nilisha-jais 's PR that is not good for a good 2021 JS code :

First of all, you are using innerHTML, this is REALLY dangerous and may cause many XSS flaws in the website. You should not use it for something like that.

Second point, you are making and array with var, this cause hoisting and it's not good for the readability and performance.

Third point, in the array, the class 'quote' is named 'quote;', so the CSS code doesn't apply to it as you call the .quote selector in the CSS file.

Fourth point, you probably took the structure of HTML in my PR, this is totally OK but there is empty tag : 'span.quote-notation' should not be empty, i removed the tag in my second PR as i just put the quote signs in the array directly. You can also delete the .quote-notation in CSS, it's not used anymore

Fifth point, using ' ` ' (template strings) in the array is not good for performance as you append 0 variables to it. It slows down the script.

And lastly, i think you should use .js file directly instead of making a script tag in the source code. It's way better for readability.

That's pretty all the mistakes i saw (I'm a Javascript dev and i'm very annoying with theses standards, i'm sorry but i had to say it x) )

You should merge just one PR at once. RN the code is working, but my js script is useless as the tag was deleted by @nilisha-jais 's PR.

I can make another PR if you want where i clean all of this. Just tell me !

PS : Theses are just advices, don't take it as an offense, i just like well written code ! :)

Neox63 commented 3 years ago

( @anubhavsingh16 )