-
Notifications
You must be signed in to change notification settings - Fork 41
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
[WEB-4087] - New Examples index page with filtering & search #2347
base: web-4082-docs-nav-redesign
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a comprehensive implementation of an Examples page with advanced filtering and search capabilities. The changes include creating multiple React components for rendering examples, implementing a dynamic filtering mechanism, and adding supporting data structures. The new implementation allows users to filter examples by products, use cases, and search terms, providing a flexible and interactive way to explore different code examples. Changes
Assessment against linked issues
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
b202981
to
437b82a
Compare
77571f2
to
40223cb
Compare
40223cb
to
99b32b3
Compare
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 12
🧹 Nitpick comments (17)
src/components/Examples/examples-checkbox.css (1)
6-6
: Remove extra space in CSS selectorThere's a double space in the selector between the
+
combinator and.ui-checkbox-styled-tick
.-.ui-checkbox-input:disabled + .ui-checkbox-styled-tick { +.ui-checkbox-input:disabled + .ui-checkbox-styled-tick {src/components/Examples/filter-search-examples.ts (2)
6-6
: Consider using consistent types for selectedProductsThe parameter type allows both
ProductName
andstring[]
, which could lead to inconsistent handling. Consider using a single type for better type safety.-selectedProducts: ProductName | string[], +selectedProducts: ProductName[],
4-19
: Consider adding input validationThe function should validate its inputs to handle edge cases gracefully.
export const filterSearchExamples = ( examples: Example[], selectedProducts: ProductName[], selectedUseCases: string[], searchTerm: string, ) => { + if (!Array.isArray(examples) || !Array.isArray(selectedProducts) || !Array.isArray(selectedUseCases)) { + return []; + } + if (typeof searchTerm !== 'string') { + searchTerm = ''; + } const normalizedSearchTerm = searchTerm.toLowerCase(); return examples.filter( // ... rest of the implementation ); };src/components/Examples/ExamplesNoResults.tsx (3)
5-5
: Fix variable naming and move data to constantsThe variable name should be plural, and the search terms should be moved to a constants file for better maintainability.
-const popularSearchTerm = ['Avatar stack', 'Live Cursors', 'Occupancy', 'Presence']; +import { POPULAR_SEARCH_TERMS } from '../../constants/examples';
4-26
: Add TypeScript interface for component propsEven though the component currently doesn't accept props, it's good practice to define an interface for future extensibility.
+interface ExamplesNoResultsProps { + // Add future props here +} -const ExamplesNoResults = () => { +const ExamplesNoResults: React.FC<ExamplesNoResultsProps> = () => {
12-16
: Use semantic HTML for list of termsThe search terms should be rendered as a list for better semantics.
-{popularSearchTerm.map((term) => ( - <Badge key={`search-term-${term}`} className="text-neutral-1300 ui-text-menu3 font-medium"> - {term} - </Badge> -))} +<ul className="flex mt-8 gap-x-6 list-none p-0"> + {POPULAR_SEARCH_TERMS.map((term) => ( + <li key={`search-term-${term}`}> + <Badge className="text-neutral-1300 ui-text-menu3 font-medium"> + {term} + </Badge> + </li> + ))} +</ul>src/components/Image.tsx (1)
10-10
: Consider documenting the relationship between 'name', 'base', and 'src' propertiesThe addition of the optional
name
property alongside existingbase
andsrc
properties could lead to confusion about when to use each. Consider adding JSDoc comments to clarify the intended usage of each property and their relationships.+ /** Unique identifier for the image, used primarily in the Examples feature */ name?: string;
src/components/Examples/ExamplesCheckbox.tsx (2)
33-33
: Simplify the onChange handlerThe arrow function wrapper is unnecessary since you're just passing the event through.
- onChange={(e) => selectProductOrUseCase(e)} + onChange={selectProductOrUseCase}
22-41
: Enhance accessibility featuresWhile the component includes basic accessibility features, consider adding:
aria-label
for better screen reader supportrole="checkbox"
for explicit semanticsaria-checked
state for custom checkbox styling<div className="flex items-center mb-0"> <input data-ui-checkbox-native="" type="checkbox" id={name} name={name} className="ui-checkbox-input" value={value} checked={isChecked || isDefaultChecked} disabled={disabled} onChange={selectProductOrUseCase} + aria-label={label} + role="checkbox" + aria-checked={isChecked || isDefaultChecked} />src/pages/examples.tsx (1)
39-53
: Add TypeScript type generation for GraphQL queryConsider adding TypeScript type generation for the GraphQL query to ensure type safety and better IDE support.
Add a type annotation for the query:
export const query: GatsbyGraphQLQuery = graphql` query ExamplesPageQuery { allFile(filter: { relativeDirectory: { eq: "examples" } }) { images: nodes { name extension base publicURL childImageSharp { gatsbyImageData } } } } `;src/components/Examples/ExamplesFilter.tsx (1)
8-24
: Props interface should be extracted for better maintainabilityConsider extracting the props interface to improve code organization and reusability.
+interface ExamplesFilterProps { + selectProduct: (e: React.ChangeEvent<HTMLInputElement>) => void; + selectUseCases: (e: React.ChangeEvent<HTMLInputElement>) => void; + handleSearch: (e: React.ChangeEvent<HTMLInputElement>) => void; + checkAllProducts: boolean; + selectedProducts: string[]; + checkAllUseCases: boolean; + selectedUseCases: string[]; +} -const ExamplesFilter = ({ +const ExamplesFilter = ({ selectProduct, selectUseCases, handleSearch, checkAllProducts, selectedProducts, checkAllUseCases, selectedUseCases, -}: { - selectProduct: (e: React.ChangeEvent<HTMLInputElement>) => void; - selectUseCases: (e: React.ChangeEvent<HTMLInputElement>) => void; - handleSearch: (e: React.ChangeEvent<HTMLInputElement>) => void; - checkAllProducts: boolean; - selectedProducts: string[]; - checkAllUseCases: boolean; - selectedUseCases: string[]; -}) +}: ExamplesFilterProps)src/components/Examples/ExamplesGrid.tsx (1)
16-31
: Move color mapping to a configuration objectThe switch statement for badge colors should be extracted to a configuration object for better maintainability.
+const PRODUCT_COLORS: Record<ProductName, string> = { + chat: 'text-violet-400', + spaces: 'text-pink-500', + liveSync: 'text-blue-600', + assetTracking: 'text-green-600', + liveObjects: 'text-green-600', +} as const; const badgeColorForProduct = (product: ProductName) => { - switch (product) { - case 'chat': - return 'text-violet-400'; - case 'spaces': - return 'text-pink-500'; - case 'liveSync': - return 'text-blue-600'; - case 'assetTracking': - return 'text-green-600'; - case 'liveObjects': - return 'text-green-600'; - default: - return 'text-orange-700'; - } + return PRODUCT_COLORS[product] ?? 'text-orange-700'; };src/components/Examples/ExamplesContent.tsx (2)
11-16
: Consider using useReducer for complex state managementThe component manages multiple related state variables that could be simplified using useReducer.
interface FilterState { selectedProducts: string[]; selectedUseCases: string[]; checkAllProducts: boolean; checkAllUseCases: boolean; searchTerm: string; filteredExamples: typeof examples.examples; } type FilterAction = | { type: 'SELECT_PRODUCT'; payload: { value: string; checked: boolean } } | { type: 'SELECT_USE_CASE'; payload: { value: string; checked: boolean } } | { type: 'SET_SEARCH_TERM'; payload: string } | { type: 'SET_FILTERED_EXAMPLES'; payload: typeof examples.examples };This would consolidate the state updates and make the component's logic more maintainable.
100-107
: Consider lazy loading decorative imagesThe StaticImage components are decorative and could be lazy loaded to improve initial page load.
<StaticImage src="./images/GridPattern.png" placeholder="blurred" width={660} height={282} alt="Grid Pattern" + loading="lazy" + aria-hidden="true" className="absolute z-0 right-0 top-64 hidden sm:block w-[60%] md:w-[40%]" />Also applies to: 109-115
src/data/examples.ts (3)
1-13
: Consider strengthening type safety with string literalsThe interfaces are well-structured, but could benefit from additional type safety:
+type SupportedLanguage = 'javascript' | 'react'; +type Product = 'spaces' | 'chat' | 'pubsub'; +type UseCase = 'multiplayer' | 'live-chat' | 'data-broadcast' | 'data-sync' | 'notifications'; export interface Example { name: string; description: string; - languages: string[]; - products: string[]; - useCases: string[]; - image: string; + languages: SupportedLanguage[]; + products: Product[]; + useCases: UseCase[]; + image: string & {}; // non-empty string } export interface Examples { examples: Example[]; - useCases: { [key: string]: { label: string } }; + useCases: { [K in UseCase]: { label: string } }; }
123-123
: Consider removing type assertionThe type assertion
as Example[]
can be avoided by properly typing the array literal.- ] as Example[], + ]: Example[],
1-141
: Consider separating types and dataFor better maintainability, consider splitting this file into:
types/examples.ts
for interfacesdata/examples-data.ts
for the actual dataconstants/use-cases.ts
for use case definitionsThis separation would make it easier to maintain and test each aspect independently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (14)
src/components/Examples/images/GridMobile.png
is excluded by!**/*.png
src/components/Examples/images/GridPattern.png
is excluded by!**/*.png
src/components/blocks/Html/__snapshots__/Html.test.tsx.snap
is excluded by!**/*.snap
src/components/blocks/wrappers/__snapshots__/ConditionalChildrenLanguageDisplay.test.js.snap
is excluded by!**/*.snap
src/images/examples/example-avatar-stack.png
is excluded by!**/*.png
src/images/examples/example-component-locking.png
is excluded by!**/*.png
src/images/examples/example-generate-token.png
is excluded by!**/*.png
src/images/examples/example-live-cursors.png
is excluded by!**/*.png
src/images/examples/example-member-location.png
is excluded by!**/*.png
src/images/examples/example-occupancy.png
is excluded by!**/*.png
src/images/examples/example-presence.png
is excluded by!**/*.png
src/images/examples/example-room-reactions.png
is excluded by!**/*.png
src/images/examples/example-typing-indicator.png
is excluded by!**/*.png
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (11)
package.json
(1 hunks)src/components/Examples/ExamplesCheckbox.tsx
(1 hunks)src/components/Examples/ExamplesContent.tsx
(1 hunks)src/components/Examples/ExamplesFilter.tsx
(1 hunks)src/components/Examples/ExamplesGrid.tsx
(1 hunks)src/components/Examples/ExamplesNoResults.tsx
(1 hunks)src/components/Examples/examples-checkbox.css
(1 hunks)src/components/Examples/filter-search-examples.ts
(1 hunks)src/components/Image.tsx
(1 hunks)src/data/examples.ts
(1 hunks)src/pages/examples.tsx
(1 hunks)
🔇 Additional comments (3)
package.json (1)
42-42
: Verify @ably/ui version update compatibility
The update from ^15.1.0 to ^15.1.9 looks reasonable as it's a minor version bump. However, let's verify the version and check for any potential issues.
✅ Verification successful
@ably/ui version update is safe to proceed
The version update from ^15.1.0 to ^15.1.9 appears safe based on:
- Extensive usage of @ably/ui components throughout the codebase, primarily core utilities and UI components
- No breaking changes detected in the usage patterns
- The codebase shows consistent integration with @ably/ui components
- The caret (^) version constraint allows for safe minor version updates
The update aligns with the PR's objective of implementing a new Examples index page with filtering & search, as evidenced by the new components using @ably/ui (ExamplesGrid.tsx, ExamplesFilter.tsx, etc.).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify @ably/ui package version and check for potential issues
# Check latest version on npm
echo "Checking latest version..."
npm view @ably/ui version
# Check for known vulnerabilities
echo "Checking for vulnerabilities..."
npm audit @ably/[email protected]
# Fetch changelog to check for breaking changes
echo "Checking changelog..."
curl -s https://raw.githubusercontent.com/ably/ably-ui/main/CHANGELOG.md | head -n 50
Length of output: 715
Script:
#!/bin/bash
# Let's check the package-lock.json for any existing version of @ably/ui
# and look for any direct dependencies or potential conflicts
# Check if package-lock.json exists and look for @ably/ui entries
echo "Checking package-lock.json..."
if [ -f package-lock.json ]; then
rg "@ably/ui" package-lock.json -A 5 -B 1
fi
# Check for any imports of @ably/ui components in the codebase
echo -e "\nChecking @ably/ui usage in codebase..."
rg "from ['|\"]@ably/ui" -A 1
# Look for any peer dependencies or related configuration
echo -e "\nChecking for peer dependencies..."
rg '"@ably/ui"' package.json -A 3 -B 3
Length of output: 12706
src/components/Examples/examples-checkbox.css (1)
1-4
: LGTM! Good use of CSS variables
The styling follows best practices by using CSS variables for colors, making it maintainable and consistent with the design system.
Also applies to: 6-8
src/data/examples.ts (1)
53-53
:
Missing image assets could affect UI rendering
Several examples have empty image properties. This might cause UI issues if the components aren't prepared to handle missing images.
Consider:
- Adding placeholder images
- Making the image property optional in the Example interface
- Implementing fallback UI for missing images
Also applies to: 77-77, 85-85, 102-102, 110-110
f0e6a1d
to
69b2787
Compare
4de27e3
to
ad94b29
Compare
1b979c6
to
a66396f
Compare
Description
A PR description indicating the purpose of the PR.
WEB-4087
Review
Instructions on how to review the PR.
Summary by CodeRabbit
New Features
ExamplesCheckbox
,ExamplesContent
,ExamplesFilter
,ExamplesGrid
,ExamplesNoResults
.Bug Fixes
@ably/ui
package dependency for improved performance and stability.Documentation
Example
andExamples
to define data structures.Style