v1.1.0 #347
Merged
v1.1.0 #347
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Update the `Dropdown` component to only call `e.preventDefault` for the up/down arrow keys if the dropdown is active. J=SLAP-2351 TEST=manual Spin up the test-site, load results, and see that the up and down arrow keys scroll the page when the dropdown is closed. When the dropdown is active, up/down arrows work as expected to navigate through dropdown options. Tested both the `SearchBar` and `FilterSearch` dropdowns.
Upgrade `ejs`, `async`, and `terser` in the test-site to address vulnerabilities in older versions of those packages. J=none TEST=none
Upgrade Jest from v27 to v29. We had [previously](#169) upgraded Jest to v28, but had to [downgrade](#275) to v27 because at the time, `@storybook/test-runner` only supported up to v27. In v0.7 of `@storybook/test-runner`, they changed Jest to be an internal dependency instead of a peer dependency, allowing us to use whatever version of Jest we want. There was only one breaking change in v29 that affected our code, besides those that already needed to be made when upgrading from v27 to v28 (see previous PR [description](#169 (comment))). This was the `jsdom` upgrade from v19 to v20 in `jest-environment-jsdom`, which requires the `typescript` version to be 4.5 or higher. With the new support added in v29 and `uuid` v9, we are now able to remove the manual resolving needed in `tests/__setup__/resolver.ts`. For more context on why this was initially needed with Jest v28, see #169. J=SLAP-2376 TEST=manual See that existing tests still run properly.
Update the run-tests workflow to have tests run on v18 of react, as well as v17 and v16.4 as it currently does. J=SLAP-2125 TEST=manual See that the tests workflow runs on react 16.14, 17, and 18 on this PR.
Fix all TS errors in the `tests` folder. J=SLAP-2360 TEST=manual Go through all files in `tests` and see that there are no more errors.
This PR is to fix the errors `Warning: Prop "id" did not match. Server: "some-id". Client: "another-id"`, reported in https://yext.slack.com/archives/C032CKFARGS/p1664916209295449 React expects that the **rendered content is identical between the server and the client**. However, our usage of `uuid` means different id is generated from the HTML content generated through SSR vs the first render in client side during hydration process. This is a known issues in React SSR (issues in react repo[[1](https://github.com/facebook/react/issues/4000)][[2](https://github.com/facebook/react/issues/5867)][[3](https://github.com/facebook/react/issues/1137)]) and React released a new hook `useId` in React 18 to address it. I found three potential solutions for React <18 to **generate unique but predictable id**, each with an external library we could use: 1) Use context to keep a counter in state, so counter is not shared across renders. This approach is implemented in [react-aria](https://www.npmjs.com/package/react-aria). When using SSR with React Aria, applications must be wrapped in an `SSRProvider` component to reset the count in server side to be zero for each fresh render. Multiple copies of React Aria is not supported so it would be a peer-dep ``` //in component lib (dropdown) import { useId } from 'react-aria'; ... const screenReaderUUID: string = useId(); //in pageJS template using SSR import { SSRProvider } from 'react-aria'; ... <SSRProvider> <TestComponent /> </SSRProvider> ``` 2) use a global counter to increment on initial renders and expose a reset function for SSR side. This approach is implemented in [react-id-generator](https://www.npmjs.com/package/react-id-generator). There's a function `resetId` required to invoke from SSR side (such as in PageJS `serverRenderRoute` or somewhere in user's template page) to avoid mismatch during refresh, since browser generator will always restart from "1" but server's count will keep incrementing. 3) Don't use IDs at all on the server; patch after first render. This approach is implemented in [reach/auto-id.](https://www.npmjs.com/package/@reach/auto-id) ID returned as undefined/empty on the first render so server and client have the same markup during hydration process. After render, patch the components with an incremented ID (second render) in useLayoutEffect. (more info [here](https://github.com/reach/reach-ui/blob/dev/packages/auto-id/src/reach-auto-id.ts)) No changes needed outside of using `import { useId } from '@reach/auto-id';` in our component lib. I decided to go with the last approach using `react/auto-id` because: - requires no additional work / changes from user side (only in component lib) - contain fallback implementation to React 18 `useId` if it's available - avoid potential concurrent rendering problem (such as suspense boundaries) - the main issue with this approach is that it causes a double render on any components with `useId`. However, since we (1) only update the ID attribute on DOM on second render, and (2) our components that uses `useId` already requires multiple renders to get setup, the performance is not greatly affected. I used Profiler in our test-site to measure performance, the number of renders and time spent for `SearchBar`, `StaticFilters`, and `FilterSearch` did not change as the dispatch from `useId` is grouped with other changes required in our components on second render. This is only needed for React <18 J=SLAP-2403 TEST=manual&auto `npm pack` a build of the component lib with the new changes and tested SearchBar, FilterSearch, and StaticFilers: - in the starter PageJS project (https://github.com/yext/pages-starter-react-locations) with React 17 - in another [yext-sites-starter](https://github.com/tmeyer2115/yext-sites-starter) repo with React 18 override See that the warning about Prop `id` mismatched no longer appear in console added new jest test for Dropdown and StaticFilter, see that test failed the previous usage of uuid, and passed with reach/auto-id.
Update the version of `@xmldom/xmldom` to address a vulnerability in Snyk. Regenerate the test-site package-lock to update some dependencies and remove other unnecessary dependencies from the top-level `search-ui-react` package. J=SLAP-2423 TEST=none
Add an export for a CSS bundle without the Tailwind preflight CSS resets. While testing, I noticed that some of our components rely on Tailwind's default border color, which is set in the CSS resets. For users who use the new bundle and specify the Tailwind resets as custom resets, the default border color isn't found because they aren't using Tailwind. I updated the components with borders that were missing a border-color so the styling is consistent regardless of what CSS resets (if any) a user provides. I also updated the default border color in the Storybook and test-site Tailwind configs to match what a no-resets user would see if we forgot to include a border-color for a border in the future. J=SLAP-2418 TEST=manual Remove Tailwind from the test-site and instead import the new bundle without Tailwind preflight. Add the Tailwind preflight CSS resets to `test-site/src/index.css` and see that the components' styling matches how they look with Tailwind (or with the CSS bundle with preflight). Scope the Tailwind CSS resets with a class name and see that only parts of the app with that class name have the CSS resets applied, while the rest of the app is unaffected.
This hotfix fixes issue #294. We had `lodash` as a devDependency, even though it's used in src. Co-authored-by: Tom Meyer <tmeyer@yext.com>
* Expose styling of facet headers in FilterGroup * Automated update to repo's documentation from github action * useMemo * move merge out of memo Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Update the versions of `@xmldom/xmldom` and `loader-utils` to address Snyk vulnerabilities in the packages. Remove `babel-loader` from the package.json because it's no longer a peer dependency for `@storybook/addon-essentials` (as of v6.5.0). J=none TEST=none
Merge main (v1.0.2) into develop
Add an `onSelect` prop to `FilterSearch` and deprecate `searchOnSelect`. If an `onSelect` function is passed in, the default selecting (and searching) logic is bypassed and we call the `onSelect`. If `searchOnSelect=true` is passed with an `onSelect`, we log a console warning that the former will be ignored. To allow for the UCSD use case mentioned in the spec, `currentFilter` was updated to allow any `StaticFilter`, not just a field value filter. J=SLAP-2431 TEST=auto, manual See that the added Jest tests pass. In the test-site, pass in an `onSelect` function that matches the current functionality with `searchOnSelect` as either true or false. Also, mimic a UCSD-like use case with compound filters and see that the component behaves correctly when selecting and un-selecting a filter. If both an `onSelect` and `searchOnSelect=true` are passed, see that a console warning is logged.
merge mapbox component feature to develop branch
Update the default behavior of `FilterSearch`:
- On select, clobber other static filters in State that have a `fieldId` that matches with one of the search fields for `FilterSearch`. If there are multiple filters that were clobbered, log a console warning.
- If the static filters state is updated from outside of the component, log a console warning if there are multiple "matching" filters in State and there is no custom `onSelect` prop passed in.
- And if there is no filter currently associated with the component, update the input text to the display name of the first "matching" filter that is selected in State.
- If there are no matching filters in State, clear out the input and filter search response.
Note: We only look for field value filters in State for the "matching" filters. We don't prescribe how compound filters should be handled when comparing one to a search field. In the UCSD use case, a developer would pass in a custom `onSelect` function that would set compound static filters in State. In this case, the `currentFilter` may not be part of the `matchingFilters` array, which is why the concept of a `currentFilter` is still needed.
J=SLAP-2432
TEST=auto, manual
See that the added Jest tests pass. Spin up the test-site and test that the above situations match the expected behavior with different combinations of filters in State and passing an `onSelect` prop or not.
Update package version to v1.1.0-beta.335.
paired with the changes in this [PR](yext/slapshot-reusable-workflows#22) in WCAG workflow, this PR updates the WCAG github action in the repo to pass in the mapbox key. Also updated the wcag test to exclude checking elements coming from mapbox canvas container as any potential violations coming from there is outside of our repo's control. WCAG github action also run on pull request to feature branch now. J=SLAP-2458 TEST=auto see that WCAG github action now passes
In addition to the changes in the [workflow PR](yext/slapshot-reusable-workflows#24), this up updates run-tests github action and coverage github action to pass mapbox api key so visual coverage test works as expected. J=SLAP-2467 TEST=auto see that run-tests github action and coverage github action passed -- looked into the logs, no false positive / errors related to visual coverage.
Update `decode-uri-component` and `loader-utils` to address Snyk vulnerabilities in older versions of those packages. J=none TEST=none
Update `FilterSearch` to account for special backend logic when searching on the `builtin.location` field. In this case, the backend can return filters on `builtin.region` or `address.countryCode` as well, so these `fieldId`s are also considered "matching" for the component. Filters on these fields can be used to populate the input and should be unselected when a new filter is selected from `FilterSearch`. J=SLAP-2495 TEST=auto, manual See that the added Jest tests pass. Spin up the test-site and add static filters on `builtin.region`. See that they are used to populate the `FilterSearch` input, warnings are logged, and the filters are unselected by `FilterSearch` as expected.
Update the `sync-sites-branch` GH workflow to remove Node 12 deprecation [warnings](https://github.blog/changelog/2022-09-22-github-actions-all-actions-will-begin-running-on-node16-instead-of-node12/) caused by `mtanzi/action-automerge` and an older version of `actions/checkout`. Since `mtanzi/action-automerge` has not been updated in a couple years, I replaced it with `devmasx/merge-branch`, which seems to be more actively maintained, is more widely used in the [GH Marketplace](https://github.com/marketplace/actions/merge-branch), and doesn't produce warnings. J=SLAP-2468 TEST=manual Test on a forked repo and see that the workflow doesn't have any warnings. The default branch of the repo is successfully synced with a test branch with a merge commit titled `Merge <default-branch> into <target-branch>`.
|
Current unit coverage is 88.8646288209607% |
nmanu1
reviewed
Dec 15, 2022
There was a problem hiding this comment.
Default behavior of
FilterSearchwas changed to better support Locators and Doctor Finders. Additionally, a newonSelectprop was added to the Component.
For this point, can we also mention that searchOnSelect is now deprecated?
Also, can we add #321 to the notes
nmanu1
reviewed
Dec 15, 2022
nmanu1
approved these changes
Dec 15, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Features
FilterSearchwas changed to better support Locators and Doctor Finders. Additionally, a newonSelectprop was added to the Component. ThesearchOnSelectprop is now deprecated. (Add onSelect prop to FilterSearch #323, FilterSearch updates forbuiltin.location#343, Update default FilterSearch behavior #333)MapboxMapComponent, powered by v2 of their JavaScript API. (Mapbox component #332)Changes
FilterGroupCssClasses. (Expose styling of facet headers in FilterGroup #321)Bug Fixes
preventDefaultonly when it is active. (Onlye.preventDefaultwhen Dropdown is active #307)