TEAMMATES / teammates

This is the project website for the TEAMMATES feedback management tool for education
https://teammatesv4.appspot.com/
GNU General Public License v2.0
1.67k stars 3.3k forks source link

CSS files: fix coding standard violations #7931

Closed amarlearning closed 6 years ago

amarlearning commented 7 years ago

Some of the CSS files are not following the CSS Coding Standard as defined by TEAMMATES here.


File Name Following Coding Standard not Followed
:white_large_square: adminAccountDetails.css Naming Standards, Order of attributes
:white_large_square: datepicker.css Order of attributes
:white_large_square: instructorCourseEnroll.css Naming Standards, Order of attributes
:white_large_square: omniComment.css Order of attributes, Hexadecimal Notation
:white_check_mark: printview.css Looks good
:white_large_square: teammatesCommon.css Naming Standards, Order of attributes, Hexadecimal Notation

damithc commented 7 years ago

Thanks for the report @amarlearning Can one of the @TEAMMATES/seniordevs investigate further and confirm?

whipermr5 commented 7 years ago

Confirmed.

amarlearning commented 7 years ago

@damithc If this is not a d.FirstTimers issue, I would like to take this one! :sweat_smile:

wkurniawan07 commented 7 years ago

@amarlearning I will caution you first that the naming standard violations will be very painful to fix.

amarlearning commented 7 years ago

@wkurniawan07 aware of that :smile:

damithc commented 7 years ago

@damithc If this is not a d.FirstTimers issue, I would like to take this one!

Not a FirstTimers issue. Can take if you want.

wkurniawan07 commented 7 years ago

@amarlearning if you feel like it, you can fix this one very blatant violation: "We only have one CSS file for the entire website".

amarlearning commented 7 years ago

if you feel like it, you can fix this one very blatant violation

While I was going through the TM CSS Coding Standard guide, I marked this ["We only have one CSS file for the entire website"] an issue.

But after reading "[In the future if we have more files, CSS file includes must be done using tags in the HTML/JSP files and NOT using @import in other CSS files.]" this, I changed my mind.

@wkurniawan07 Do you really think that's an issue?

wkurniawan07 commented 7 years ago

It is. There is no clear separation on which files need to be separated right now, and at the current state such separation creates more confusion than benefits (it already happens right now, am I right?).

amarlearning commented 7 years ago

and at the current state such separation creates more confusion than benefits

@wkurniawan07 exactly. I think we should divide the CSS into three sections Admin, Instructor, Student. This way we can minimize the loading time of every page because the file size will not be too big.

If we combine all the CSS into one file i.e. teammatesCommon.css, then the CSS file size will be too high which will affect the loading time of web pages.

Let me know what you think

wkurniawan07 commented 7 years ago

If we combine all the CSS into one file i.e. teammatesCommon.css, then the CSS file size will be too high which will affect the loading time of web pages.

Only the initial loading time will be affected. All subsequent loads will simply load cached CSS file.

amarlearning commented 7 years ago

Only the initial loading time will be affected. All subsequent loads will simply load cached CSS file.

@wkurniawan07 great :+1:

I want to divide this issue into three seperate PRs:

  1. For fixing the coding standards of adminAccountDetails.css, datepicker.css, instructorCourseEnroll.css and omniComment.css

  2. For fixing the coding standards of teammatesCommon.css.

  3. To fix "We only have one CSS file for the entire website"

Let me know what you think

wkurniawan07 commented 7 years ago

@amarlearning I'd rather you fix everything not naming related in one PR first, then fixing all naming violations in at least one PR. If you're familiar enough with the code base you would understand why.

whipermr5 commented 7 years ago

Effort tracker, to be updated as each PR is merged:

PR Effort
#7969 2
#8352 4