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

Cleanup and improvements #208

Merged
merged 6 commits into from Jan 2, 2022
Merged

Cleanup and improvements #208

merged 6 commits into from Jan 2, 2022

Conversation

therealsujitk
Copy link
Collaborator

@therealsujitk therealsujitk commented Dec 23, 2021

Hi @vatz88 I made quite a few changes in this PR but don't worry you don't have to look at the code, I'll list down the things I've changed below.

  • Moved SCSS rules into separate files and removed rules no longer in use. In the past you were hesitant to use custom CSS because it might not have cross browser support, so I've moved as much as I could to bootstrap and now there's barely any custom CSS (I'm surprised too).
  • Moved JS functions into separate files, removed redundant and deprecated code. You were concerned with how I was storing a bunch of data and functions in the window object, I googled about that and looks like you were right, while requiring a package in nodejs as long as it's in the same directory it always gives the same instance. However I think Bootstrap 4 requires jQuery to be a window object because of how it access' it, one of the packages we use requires BS4 for now, however I might be able to resolve this once they release a stable version with BS5 support.
  • I've added separate last update fields for vellore and chennai for obvious reasons and also moved the last updated semester to a more viewable location. This value changes when the semester is changed.
  • I've changed the ascending and descending arrows to use FontAwesome icons instead making it more UI friendly. Also, I changed ascending to an up-arrow and descending to a down-arrow (it was reversed), after someone mentioned it in the past I looked around and found people saying the pointed part is the smaller part. If you want me to change it back let me know.
  • And finally the x-axis padding in the timetable and course list, you wanted them to be full width but it didn't look very good on larger devices, so I've added a breakpoint to use full width on smaller devices only (personally I think it looks better if smaller devices had a small amount padding as well but this works too).

If you have any suggestions please let me know, I won't be merging this PR immediately (Edit: It looks like I don't have permission to merge my own PRs). There's still a tiny amount of cleaning left. I'll merge it after I resolve #169. I did this cleanup first so that everything would be more organised for the major changes I'll have to do to support different timetables.

This PR closes most of #193.

@therealsujitk therealsujitk marked this pull request as draft December 23, 2021 06:16
@therealsujitk
Copy link
Collaborator Author

Also, I'd like to increase the required node version for this application to 14 or some other higher LTS version. Node.js 12 will be reaching end of life soon.

@therealsujitk therealsujitk marked this pull request as ready for review December 24, 2021 18:05
Hiding the text in the option buttons if the screen
width is not big enough
*/
button span {
Copy link
Owner

Choose a reason for hiding this comment

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

button#option-buttons span ?
To be more specific

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I'll add this.

@vatz88
Copy link
Owner

vatz88 commented Dec 30, 2021

Sounds good to me!

@vatz88
Copy link
Owner

vatz88 commented Dec 30, 2021

Should the download course list be a separate button somewhere in course panel?

image

"Download timetable" is not very obvious to have two options.

@vatz88
Copy link
Owner

vatz88 commented Dec 30, 2021

Should the download course list be a separate button somewhere in course panel?

image

"Download timetable" is not very obvious to have two options.

Ah my bad, i confused it with #209

@therealsujitk
Copy link
Collaborator Author

therealsujitk commented Dec 30, 2021

"Download timetable" is not very obvious to have two options.

I can't think of calling it anything else. I had previously planned to add a third option in that modal where users can download a PDF containing both of them. (Might add it in the future, the code is somewhere in my fork)

Should the download course list be a separate button somewhere in course panel?

I'll try moving things around and see if something looks good.

Edit:

Ah my bad, i confused it with #209

Oh, alright.

@therealsujitk
Copy link
Collaborator Author

therealsujitk commented Dec 30, 2021

Also, could you unlink the issue this PR is linked to, It got linked automatically when when I mentioned it. It should be linked to #193. Thanks!

@vatz88
Copy link
Owner

vatz88 commented Dec 30, 2021

GitHub isn't letting me 🤦‍♂️
Also, you're added as contributor to this repo so if I can you can as well, or we both can't

@therealsujitk
Copy link
Collaborator Author

therealsujitk commented Dec 30, 2021

GitHub isn't letting me 🤦‍♂️

Oh, I thought I didn't have permission to do it. Must be a GitHub bug.

Edit: Yeah, it's not letting my remove the first one it linked for some reason.

@therealsujitk therealsujitk linked an issue Dec 30, 2021 that may be closed by this pull request
14 tasks
@therealsujitk therealsujitk merged commit e310cab into vatz88:master Jan 2, 2022
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.

Minor improvements and cleaning Support different timetables for different campuses
2 participants