feat(filter): added filter common component #1027
Conversation
✅ Deploy Preview for activist-org ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Thank you for the pull request!The activist team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :) If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Development rooms once you're in. Also consider joining our bi-weekly Saturday dev syncs. It'd be great to have you! Maintainer checklist
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
First PR Commit Check
- The commit messages for the remote branch of a new contributor should be checked to make sure their email is set up correctly so that they receive credit for their contribution
- The contributor's name and icon in remote commits should be the same as what appears in the PR
- If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for
git config user.emailin their local activist repo
|
I'll be removing the filter component usage from organizations before merging. It is currently there for reviewer to visualize how the filter component looks. @andrewtavis please feel free to check it out and leave any necessary comments. Thanks! |
|
Looking forward to the review here, @Hasteerp! I'll try to get to this tomorrow or early next week 😊 |
|
@Hasteerp We just added component testing to this project. Could you try adding some tests for your new filter component? There's some new documentation on writing component tests in the contributing guide you can find here. This is new so let us know if the documentation is clear enough or if it needs improvements. |
|
@cquinn540 I was having a difficult time getting the test to run. Would you have any suggestions? |
I would need to know more about what issue you are facing. I'll be a bit busy today, but I should have more free time to help over the next few days. |
|
@cquinn540 I was facing some issues trying to get my component to render I think the custom render function needs to change but I am not exactly sure. This is the error message that I got. Please feel free to check it out thanks! `This error originated in "test/components/filter/pageFilter.spec.ts" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
|
|
@Hasteerp Would you have time today or sooon to get on a call in Matrix chat so I can see what you are seeing? That's the top level error message which is generic and doesn't give much information. There's usually another stack trace somewhere in the output with more details. |
|
I can get on a call tomorrow @cquinn540 if that works for you. |
|
Sorry @Hasteerp I got into the zone working on another issue and didn't see this until now. Let me know on the Matrix chat when you are available today. |
|
@andrewtavis @cquinn540 Please check it out whenever you can. I added the test case. |
| import { describe, it, expect } from "vitest"; | ||
| import { render, screen } from "@testing-library/vue"; | ||
| import PageFilter from "@/components/page/filter/PageFilter.vue"; | ||
|
|
There was a problem hiding this comment.
Okay, I need to go back on what I said originally a little. There is an appropriate case for a mock function here. You should create a mock function like:
const mockHandleFilterChange = vi.fn();and you'll want to reset it after each test
afterEach(() => {
mockHandleFilterChange.mockClear();
});| tabs: [ | ||
| { id: "tab1", name: "Tab 1" }, | ||
| { id: "tab2", name: "Tab 2" }, | ||
| { id: "tab3", name: "Tab 3" }, |
There was a problem hiding this comment.
Then you should use mockHandleFilterChange as the @filter-change prop
| ], | ||
| }; | ||
|
|
||
| it("should render", () => { |
There was a problem hiding this comment.
Then you want a test where you click the different options and confirm that mockHandleFilterChange was called with the right arguments in the right order.
And another test where you tab through the options and hit enter and again confirm the mock was called correctly.
You can see the call list with calls array on your mock function.
Ideally you would also have a test checking that the focusing works correctly, but given your time limit I can be flexible on that.
| expect(screen.getByText("Popular Tags")).toBeTruthy(); | ||
| expect(screen.getByRole("heading", { name: "Filter" })).toBeTruthy(); | ||
| expect(screen.getByLabelText("Search to filter")).toBeTruthy(); | ||
| expect(screen.getByRole("textbox", { id: "input-search" })).toBeTruthy(); |
There was a problem hiding this comment.
With the other behavior tests testing for truthyness shouldn't be needed anymore
|
@andrewtavis thanks for the pull request and allowing me to merge this one. We are currently seeing an error in linting due to unused vars (this is coming from index.vue on organizations page). This is because we commented the code out but the parameters were defined. We can either remove them completely or comment out the code for now. Please let me know what you would suggest I do and I can make a commit accordingly. Thank you for taking into consideration my deadline. Appreciate it! I'll work on the test cases in the new PR in the coming week. |
There was a problem hiding this comment.
Approving these with the assumption that the most recent commit fixes all the frontend linting issues, but I'll get to them if there are any other changes.
Really appreciate this as a first contribution, @Hasteerp, and hope that we can work with you again :) Let's definitely keep #1051 in mind in the coming weeks, but this is a great start on a relatively complex component 🚀
@cquinn540: Don't mean to disregard your review, and really appreciate the support you're providing to @Hasteerp! This component isn't MVP, so under the circumstances I'd like to bring it in and iterate 😊
|
No problem this seems like a reasonable compromise |
Contributor checklist
Description
I added a common reusable Filter component.
The Filter component is a reusable component.
It takes the following props:
This way you have full control over the tag title and tag buttons you want to populate. Additionally, decision can be made to check tab values and name based on each page requirement. It also takes handleFilterChange from the parent component that allows different filteration logic based on parent component requirement.
Related issue