-
-
Notifications
You must be signed in to change notification settings - Fork 778
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
Implement filter on wins page (#1030) #1034
Implement filter on wins page (#1030) #1034
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akibrhast - Really nice work! I've just done a quick look, but the code looks really clean and the functionality looks like it works as it should.
A couple of nips and tucks:
- At a certain point, on mobile, the "Team" dropdown floats off the screen
I'm not sure what the solution to this is, probably that the size should just get smaller ('cause you can't really stack them (Can you?)). This is the one that really needs to be fixed before we merge (in my opinion).
- The box on the dropdown is wider than the box before it drops down, and it pushes out over the bottom of the other drop-down:
It does that on the current frontpage, so maybe that's a feature and not a bug. I find it sub-optimal, but...what do I know.
-
On Firefox there's a weird thing that happens where, if I tell it to load an iPhone 6 (or anyother mobile size) screen, the drop-downs don't work. If I drag the screen to desktop, and then drag it back to the exact same size, they work fine. I'm not sure if that means anything, or if it's just a browser anomaly, so I'm just mentioning it. (There's a way to check this on an actual phone; I'll see if I can remember how to do that).
-
On Firefox, the dropdowns open and close with a mouseover. In chrome, they open with a click, which is fine. They close if I click outside the box, or on the other dropdown. I clicked on the arrow in the dropdown a bunch of times thinking "why isn't this closing?!" before it occurred to me that maybe they weren't supposed to. Should we have the drop-down close on click also?
@daniellex0 - Most of these are actually design questions. Can you weigh in? Thanks.
Looks good! -2. Yes, I believe this is because some of the items within the homepage dropdown are longer than the top box so they need more room. It would look cleaner if it was the same size, but this does leave some room for longer team names in the future.. I think for the sake of standardization we can leave it like this for now instead of creating a new class with a different size right now, since this works and looks good enough- but maybe down the line I can add a few different sizes of dropdown options to the design system. -4. The dropdowns open and close for me when I scroll over on Chrome! Btw just a small type-o @akibrhast - (under the div with classname .wins-page-contain projects-inner) - it says id="filter-template-conatainer |
@daniellex0 - What do you think about question 1, where half of the team drop-down is cut off on mobile. What a good fix for that? |
@akibrhast @jbubar - Just a general question for this, and all of the other pages that we work on. Is there any javaScript on this page that might be useful on other pages that we should put in a file to be included, so it can be reused rather than having to be re-written on every page we use it on? For example, since the drop-downs are basically the same on this page and on the homepage, is there any of that code that is re-useable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really really awesome! I really enjoyed review this code. I learned a lot of new methods and functions! thanks Akib! It works great. One thing is that, as a project, we do not use inline styling. But I think that this can be separated out into a medium sized issue. We will talk about it in the dev meeting today
@jbubar @drubgrubby The filters that are currently being used in wins-page.html is very similar in what it is doing when comparing to the filter implemented in project-filter.js The comment for the class defined in project-filter.js states
As such it should be possible to create a singular class that can handle that, or maybe subclass from the existing function. Maybe make a new issue/discussion about |
@drubgrubby I think it should be formatted the exact same way the dropdowns on the homepage are in mobile if possible - |
Made changes as requested by @drubgrubby
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small changes. use new classes, and change the breakpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akibrhast - Really good work! This is ready to merge.
Fixes #1030
This PR Implements the filtering system on the wins pages.