Filter courses by location

[Anjan-11]

Issue 1:It would be great if you could add an option to filter the buildings in the filter option for eg: TT, SJT.
Thanks for the amazing website.

[therealsujitk]

I'll be able to take this up @vatz88, can you assign this to me? I'll work on it sometime this month. The .xlsx sheets already have a VENUE column, so it's basically copying and pasting stuff, but i'd like to make some minor improvements apart from this (related to this issue). I'll discuss about the changes i'm thinking of right here before starting anything.

[vatz88]

Sure @therealsujitk ! Go ahead. We already have solt filter available. Maybe you can do something on similar lines. And yes, improvements are always good. Feel free to discuss here, I'll be happy to help.

Thanks for taking this up :D

[therealsujitk]

@vatz88 as of now there are two separate fields, one specifically for the course code and another for the course title. I was thinking why not merge them? It would look something like this.

This would also give space for more filters.

[vatz88]

Hey @therealsujitk this looks good! Less input fields and more space is always better. We can do this, more convenient than two fields and no compromise with the features or usability

[therealsujitk]

In this code snippet, I noticed you are checking if a value key is present and if it isn't you are iterating through a sub object assuming that the selected option was all slots. However there isn't an all slots option, were you hoping for that feature to be implemented in future? if so, i'll do the same thing for venues.

for (var key = 0; key < option.length; key++) {
    if (option[key.toString()].value) {
        filterSlotArr.indexOf(option[key.toString()].value) ===
            -1 && filterSlotArr.push(option[key.toString()].value);
    } else {
        var allSelectOption = option[key.toString()];
        for (
            var innerkey = 0;
            innerkey < allSelectOption.length;
            innerkey++
        ) {
            if (allSelectOption[innerkey.toString()].value) {
                filterSlotArr.indexOf(
                    allSelectOption[innerkey.toString()].value,
                ) === -1 &&
                    filterSlotArr.push(
                        allSelectOption[innerkey.toString()].value,
                    );
            }
        }
    }
}

Also, is iterating through the option object necessary? I've seen it always has only one key, 0.

[vatz88]

I honestly don't remember most of the code because it's been very long since i wrote it. You should feel free to refactor it but make sure all edge cases are covered.

I think one of the assumptions is that if no slot is selected in the slot filter, we show all the course for all the slots (think it as an implicit all slots option). If we don't do this, since no slot is selected initially, no courses will be displayed. If by default we keep all slots selected, then it's rather tedious to unselect all before you can apply filter on one slot.

[therealsujitk]

I've done pretty much the same thing for the venue filter. I'm almost done with this, I just want to make some changes to the local datasheet to test if it's working as it should. As this issue is pretty much complete, i'll open a new issue for adjustments, improvements and some code cleaning before the next release as it isn't related to this issue.

This is how it looks with the new filter, i'll open a pull request as soon as i'm done with some local testing.

I wanted to try and implement #76 where users can select a slot or venue and get a list of all the courses, but after looking at the code I realised it would be completely different from this issue and a lot of changes will have to be made. Well I have 3 more years to get to it so maybe someday 🤷‍♂️.

Edit: It works.

[therealsujitk]

@vatz88 just confirming, what i've done is filter according to the venues provided, for example it filters AB1 - 101 or AB1 - 102. It doesn't give you all the courses in AB1. Was I supposed to filter them according to the building names? If so, it shouldn't take long, I can do that.

[vatz88]

filter are applied on top of course which is being searched, so this should be good

@vatz88 just confirming, what i've done is filter according to the venues provided, for example it filters AB1 - 101 or AB1 - 102. It doesn't give you all the courses in AB1. Was I supposed to filter them according to the building names? If so, it shouldn't take long, I can do that.

Thanks for the PR, I'll review it in my free time. Make sure you have tested all edge cases. Also, test to make sure new changes are good wrt mobile too, mostly layout and responsiveness.

For more PR on improvements/ code clean up, you can create new issues for the same by yourself

[therealsujitk]

filter are applied on top of course which is being searched, so this should be good

Right now it'll filter out the buildings along with the rooms in it, but I think what the author was asking for was to filter them according to the buildings alone.

For example if CSE1001 is available in AB1 - 101, AB1 - 303 & AB2 - 204, the current filter will show AB1 - 101, AB1 - 303 & AB2 - 204. However I think the author wants the filter to show AB1 & AB2. I feel the current filter is a bit pointless, it'll be rare for two slots in a course to have the same building and room.

If you want me to implement this I can, however I'll need to know how the venues are formatted, I'm a first year and have never been to college so I don't know if it's of the form BUILDING - ROOM in Chennai or not, and I think it's different for Vellore. If there's a proper format (even if it's different for Chennai & Vellore), I'll be able to do it.

[vatz88]

I feel the current filter is a bit pointless, it'll be rare for two slots in a course to have the same building and room.

What you're saying makes sense. In the current excel sheet, I don't see venues present. Plus we can't expect VIT to provide us with consistent format for venue. How about for now we merge the change of combined input for course code and title and let venue thing be for later?
Can you raise PR with only that one change? I'll merge it

[therealsujitk]

Can you raise PR with only that one change? I'll merge it

Sure! I'll close the current one and open a new pull request with the merged inputs alone.