Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

feat: improving icons page #9655

Merged

Conversation

sital002
Copy link
Member

Fixes Issue

closes #9625

Changes proposed

Added the searched icons on the URL as query params so that we can share the link to list all the available filtered icons

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

After changes:
image

Note to reviewers

@github-actions github-actions bot added the issue linked Pull Request has issue linked label Oct 28, 2023
Copy link
Member

@eddiejaoude eddiejaoude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 👍

Few items...

  • Please resolve conflict, I think you can accept the changes coming in
  • The failing test can be fixed with these changes below (you can add these changes via a patch file, by saving it to a file and use git apply <file-name>
diff --git a/tests/icon.spec.js b/tests/icon.spec.js
index 6485b9fe..62e84310 100644
--- a/tests/icon.spec.js
+++ b/tests/icon.spec.js
@@ -1,5 +1,6 @@
 // @ts-check
 import { test, expect } from "@playwright/test";
+import AxeBuilder from "@axe-core/playwright";
 
 const defaultIcons = 20;
 
@@ -17,7 +18,7 @@ test("Icon search works correctly", async ({ page }) => {
 
   // 3. type in search and check that icons with the name exist and check a name doesn't exist
   const input = page.locator("[name='keyword']");
-  await input.type("mobile");
+  await input.fill("mobile");
   const results = await page.locator("main ul li").count();
 
   await expect(results).toBeGreaterThanOrEqual(7);
@@ -29,7 +30,7 @@ test("Icon search page has default results when no search term used", async ({
   await page.goto("/icons");
 
   const input = page.locator("[name='keyword']");
-  await input.type("");
+  await input.fill("");
 
   await expect(page.locator("main ul li")).toHaveCount(defaultIcons);
 });
@@ -40,7 +41,7 @@ test("Icon search page shows default results after typing 1 characters", async (
   await page.goto("/icons");
 
   const input = page.locator("[name='keyword']");
-  await input.type("e");
+  await input.fill("e");
 
   await expect(page.locator("main ul li")).toHaveCount(defaultIcons);
 });
@@ -51,9 +52,34 @@ test("Icon search page shows results after typing 3 characters", async ({
   await page.goto("/icons");
 
   const input = page.locator("[name='keyword']");
-  await input.type("hand");
+  await input.fill("hand");
+  await expect(page).toHaveURL("/icons?keyword=hand");
   const results = await page.locator("main ul li").count();
 
   await expect(page.locator("main ul li")).toContainText(["hand"]);
-  await expect(results).toBeGreaterThanOrEqual(defaultIcons);
+  expect(results).toBeGreaterThanOrEqual(defaultIcons);
+});
+
+test.describe("accessibility tests (dark)", () => {
+  test.use({ colorScheme: "dark" });
+
+  test("should pass axe wcag accessibility tests (_test-profile-user-6) (dark)", async ({
+    page,
+  }) => {
+    await page.goto("/_test-profile-user-6");
+    const accessibilityScanResults = await new AxeBuilder({ page })
+      .withTags(["wcag2a", "wcag2aa", "wcag21a", "wcag21aa"])
+      .analyze();
+    expect(accessibilityScanResults.violations).toEqual([]);
+  });
+
+  test("should pass axe wcag accessibility tests (_test-wcag-user) (dark)", async ({
+    page,
+  }) => {
+    await page.goto("/_test-wcag-user");
+    const accessibilityScanResults = await new AxeBuilder({ page })
+      .withTags(["wcag2a", "wcag2aa", "wcag21a", "wcag21aa"])
+      .analyze();
+    expect(accessibilityScanResults.violations).toEqual([]);
+  });
 });

@sital002 sital002 marked this pull request as draft October 31, 2023 10:56
@sital002
Copy link
Member Author

sital002 commented Nov 3, 2023

I think rather than searching onChange we should wrap this in a form and search and add to query params onSubmit because updating the query params onChange will rerender the whole page every time the input changes which is not a good experience I proposed the same solution for search page as well.

@eddiejaoude
Copy link
Member

I think rather than searching onChange we should wrap this in a form and search and add to query params onSubmit because updating the query params onChange will rerender the whole page every time the input changes which is not a good experience I proposed the same solution for search page as well.

I think it is a good idea also, but can you link to the search page proposal, is there an example there?

@sital002
Copy link
Member Author

sital002 commented Nov 9, 2023

I think it is a good idea also, but can you link to the search page proposal, is there an example there?

Sure here is the link for it #9248 (comment)
This will resolve the fluctuation error on search page since it won't rerun the useEffect onChange

@eddiejaoude
Copy link
Member

Thank you 👍

@sital002 sital002 marked this pull request as ready for review November 9, 2023 09:46
@sital002 sital002 requested a review from eddiejaoude November 9, 2023 09:46
@sital002 sital002 force-pushed the feat-icons-page-search branch from 8313e25 to d89e369 Compare November 9, 2023 09:59
@sital002
Copy link
Member Author

sital002 commented Nov 9, 2023

The failing test can be fixed with these changes below (you can add these changes via a patch file, by saving it to a file and

I am unable to add this test it says corrupt at line 75

@eddiejaoude
Copy link
Member

Oh strange, I can't make changes to your fork, but I can merge to a temporary branch and apply the test changes

@eddiejaoude eddiejaoude changed the base branch from main to fix-pr-9655 November 9, 2023 10:58
@eddiejaoude eddiejaoude merged commit abb4a30 into EddieHubCommunity:fix-pr-9655 Nov 9, 2023
@eddiejaoude eddiejaoude mentioned this pull request Nov 9, 2023
6 tasks
eddiejaoude added a commit that referenced this pull request Nov 11, 2023
* feat: improving icons page  (#9655)

* feat:refactor icons page

* add ref in input element

* refactor icons page

* fix: icon search button

* fix: update tests

* fix: icon test to click button

* Update pages/icons.js

Co-authored-by: sital002 <[email protected]>

* Update pages/icons.js

---------

Co-authored-by: sital002 <[email protected]>
@sital002 sital002 deleted the feat-icons-page-search branch December 1, 2023 08:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue linked Pull Request has issue linked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] improving icon search
2 participants