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

FFCS Redesign #179

Closed
wants to merge 26 commits into from
Closed

FFCS Redesign #179

wants to merge 26 commits into from

Conversation

therealsujitk
Copy link
Collaborator

@therealsujitk therealsujitk commented Jul 12, 2021

This pull request contains the FFCS redesign.


  • The Filter By Slots option has to be fixed.
  • For some reason, the window doesn't scroll to the top before taking a screenshot. Screenshots may get cut because of this. (The scroll to the top looks like a hack, an alternative should be implemented).
  • Dark theme is incomplete. (Will work on this once the current UI for light mode has been frozen). A new PR will be created for this.

Note: Unused resources might be found in this PR, I'm planning to clean that up in the next PR.

@vatz88
Copy link
Owner

vatz88 commented Jul 13, 2021

Thanks @therealsujitk ! Looks great!

I'll review the code but can we make few changes:

  1. Let hide the theme toggle button for now, we can enable it when we have dark mode added (may be in a separate PR)

  2. I feel these two texts in pink is too bright. Can give it a more subtle? This isn't the main text on the site, it's just like a tip
    image
    image

  3. This looks very cluttered, if you compare it with what we have currently. Need to improve on it, especially spacing
    image

@therealsujitk
Copy link
Collaborator Author

Let hide the theme toggle button for now, we can enable it when we have dark mode added (may be in a separate PR)

Alright! Will do.

I feel these two texts in pink is too bright. Can give it a more subtle? This isn't the main text on the site, it's just like a tip

I thought it matched with the sites theme. I'll try some lighter colours and see.

This looks very cluttered, if you compare it with what we have currently. Need to improve on it, especially spacing

Alright, I'll make adjustments.

@vatz88
Copy link
Owner

vatz88 commented Jul 13, 2021

I general, we should minimize writing custom CSS and let defaults of bootstrap be used

@therealsujitk
Copy link
Collaborator Author

therealsujitk commented Jul 13, 2021

I've used bootstrap as much as I could, the theme toggle (now hidden) wasn't a feature in bootstrap, so that was a custom addition. Apart from that I've only used CSS to make margin, padding adjustments or to override the default settings.

There are blockquotes in bootstrap, but they are just text with a bigger font size, and I didn't think that matched with the rest of the site.

@therealsujitk
Copy link
Collaborator Author

@vatz88 are the quick selection buttons alright now?

@therealsujitk
Copy link
Collaborator Author

therealsujitk commented Jul 13, 2021

@vatz88 how does lavender (#ddbbff) look?

localhost_1234_ (3)


We'll have to change the pink used in the slot tiles as well, doesn't go good with lavender. Also I think it'll look good if we give a black border to the course table.

@vatz88
Copy link
Owner

vatz88 commented Jul 13, 2021

how does lavender (#ddbbff) look?

I'd say may be keep it same as the current one. It doesn't need any bright color. Maybe just white bg and grey text is good.

Also I think it'll look good if we give a black border to the course table

Yes! we need it as in the current design for it to look more tidy

@vatz88
Copy link
Owner

vatz88 commented Jul 13, 2021

are the quick selection buttons alright now?

Yes, better with spacing now

@therealsujitk
Copy link
Collaborator Author

I'd say may be keep it same as the current one. It doesn't need any bright color. Maybe just white bg and grey text is good.

Oh, didn't think of that. Let me see.

@therealsujitk
Copy link
Collaborator Author

therealsujitk commented Jul 14, 2021

@vatz88 I made the background light grey to make it stand out a little, fixed the scroll to top issue that I mentioned in the first post & added a black border to the course table.

@vatz88
Copy link
Owner

vatz88 commented Jul 14, 2021

Let's make timetable take the full width as it was with container-fluid.
Margin/padding on the sides is space wasted especially on smaller devices

image

@vatz88
Copy link
Owner

vatz88 commented Jul 14, 2021

Can we give max-width to these course buttons? So that the one in the bottom doesn't feel odd

image

@therealsujitk
Copy link
Collaborator Author

Let's make timetable take the full width as it was with container-fluid.

I actually tried that at first, but it looked very odd, the only element with no margin. It felt like it was a bug. I was able to use it comfortably on my phone so I thought it should work, plus the small margin doesn't make much of a difference. Have you tried using it on a phone yet? If you feel that it should still be changed then I'll do it, but I feel the current one is best.

Can we give max-width to these course buttons? So that the one in the bottom doesn't feel odd

If we give it a max-width, that would cause empty gaps because flex-grow makes sure that everything in the row has the same width. It is similar to the current design that is implemented.

localhost_1234_ (5)

I do agree with you tho, this does look better (except for the extra spacing). I'll work on this and see if I can make it so the last row alone doesn't grow fully.

@therealsujitk
Copy link
Collaborator Author

therealsujitk commented Jul 14, 2021

@vatz88, I got what you asked for using grid instead of flex.

deploy-preview-179--ffcsonthego netlify app_

@therealsujitk therealsujitk marked this pull request as ready for review July 14, 2021 17:37
@therealsujitk
Copy link
Collaborator Author

@vatz88 before merging this PR, can you archive the current site, maybe at https://ffcsonthego-archive-2.vatz88.in/ or something similar. That way it can be added to the README.md.

@vatz88
Copy link
Owner

vatz88 commented Jul 15, 2021

Hey @therealsujitk thanks for all the efforts! I'll try it out myself and go through the code (probably over weekend) and make any changes needed before merging.

I'll have to check how well is grid supported in browsers other than chrome, especially safari (for iphone users)

One more change needed is we need to add some border to the course search panel as in the current design. It's all open in white and not distinctive.

@therealsujitk
Copy link
Collaborator Author

therealsujitk commented Jul 15, 2021

I'll have to check how well is grid supported in browsers other than chrome, especially safari (for iphone users)

Alright! I'll start working on the clean up PR on top of this branch.

Edit: I'll work on the clean up PR after this one is merged (on top of master), otherwise git will include these changes in that PR and it'll be a lot harder to review.

One more change needed is we need to add some border to the course search panel as in the current design. It's all open in white and not distinctive.

Not sure if a border will look good, but I'll try and see if something looks good. I had mentioned before that bootstrap removed their panel, so that isn't an option anymore.

I've divided the site into 3 main sections #1 - Course Panel, #2 - Timetable (#2 includes the course list) & #3 - Comments (#3 will most likely never have to be changed) all separated by a hr tag. I did this so that the files can be separated into these 3 main sections as well (Ex: course-panel.js, timetable.js) and contributors will know exactly where to look to resolve an issue (I'll also add what files are connected to what section in the CONTRIBUTING.md).

@vatz88
Copy link
Owner

vatz88 commented Jul 15, 2021

before merging this PR, can you archive the current site

We don't need that. I'll make a new release on this repo. I have the old site deployed on different domain because it used facebook comments and I didn't want the old comments/ discussions to be lost

@therealsujitk
Copy link
Collaborator Author

Oh ok, understood!

I'll make a new release on this repo.

After merging, can you wait a bit for the release. There are some small things I'd like to add in the clean up PR such as,

  • Instead of changing the version in the index.html file, I thought it'll be a lot cleaner to add a release key in the package.json file and read that value using JS. That way you'd only need to change the version name in the package.json file for every release.
  • Some code optimising. Ex: The time complexity for the slot filter is currently O(n2) as it iterates through all the buttons and all the the elements in the slot array. I believe I can bring this down to O(n).

Apart from this, the current timetable is not up-to-date (#169). So that will require another PR.


I'll start working on the clean up PR as soon as this one is merged (on top of master). Otherwise these changes will be included in the clean up PR and it'll be a lot harder to review.

@vatz88
Copy link
Owner

vatz88 commented Jul 15, 2021

Deployments are continuous with netlify once the pr is merged to master. The release I'll make will be here https://github.com/vatz88/FFCSonTheGo/releases

Probably a major release with v11 after this PR is merged. Future minor fixes/changes can go as like v11.1 and so on

@vatz88 vatz88 closed this in #181 Jul 20, 2021
vatz88 added a commit that referenced this pull request Jul 20, 2021
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.

None yet

2 participants