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

Filter courses by location #144

Open
Anjan-11 opened this issue Jul 7, 2020 · 12 comments
Open

Filter courses by location #144

Anjan-11 opened this issue Jul 7, 2020 · 12 comments
Labels
enhancement New feature or request priority/low This issue doesn't have to be resolved immediately

Comments

@Anjan-11
Copy link

Anjan-11 commented Jul 7, 2020

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.

@vatz88 vatz88 added Available enhancement New feature or request labels Jul 7, 2020
@vatz88 vatz88 changed the title Building filter in filter Filter courses by location Jul 7, 2020
@therealsujitk
Copy link
Collaborator

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
Copy link
Owner

vatz88 commented Jul 2, 2021

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
Copy link
Collaborator

@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.

Screenshot from 2021-07-03 13-05-23

Screenshot from 2021-07-03 13-05-47

This would also give space for more filters.

@vatz88
Copy link
Owner

vatz88 commented Jul 3, 2021

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
Copy link
Collaborator

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
Copy link
Owner

vatz88 commented Jul 3, 2021

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
Copy link
Collaborator

therealsujitk commented Jul 3, 2021

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.

image

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.

image

@therealsujitk
Copy link
Collaborator

therealsujitk commented Jul 3, 2021

@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
Copy link
Owner

vatz88 commented Jul 3, 2021

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

@vatz88 vatz88 linked a pull request Jul 3, 2021 that will close this issue
@therealsujitk
Copy link
Collaborator

therealsujitk commented Jul 3, 2021

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
Copy link
Owner

vatz88 commented Jul 5, 2021

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
Copy link
Collaborator

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.

@therealsujitk therealsujitk added the priority/low This issue doesn't have to be resolved immediately label Dec 11, 2021
@therealsujitk therealsujitk removed their assignment Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority/low This issue doesn't have to be resolved immediately
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants