Fix pagination component render condition for responsive breakpoint behavior#1062
Fix pagination component render condition for responsive breakpoint behavior#1062
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
Co-authored-by: lucasoares <10624972+lucasoares@users.noreply.github.com>
Co-authored-by: lucasoares <10624972+lucasoares@users.noreply.github.com>
|
|
@copilot can you create a storybook with examples of each state of the pagination component? |
Co-authored-by: lucasoares <10624972+lucasoares@users.noreply.github.com>
I've created comprehensive Storybook examples showing all pagination component states including:
These examples demonstrate all the different configurations and behaviors of the pagination component. Commit: 1d2988b |
|
@copilot check last test run on pr checks and fix the test |
|
@copilot about the storybook, can you put all in a single storybook page? Is there a way to have a button to see the code of each storybook example? Also, the "Without Page Counter" example is correct? It is showing:
|
Co-authored-by: lucasoares <10624972+lucasoares@users.noreply.github.com>
Fixed the failing pagination responsive tests. The issue was with circular structure JSON serialization when passing E2EElement objects to Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: lucasoares <10624972+lucasoares@users.noreply.github.com>
I've consolidated all Storybook examples into a single "AllStates" story that shows all pagination configurations organized by sections. The story is configured to show source code by default with I also fixed the "Without Page Counter" NaN issue. The problem was that Commit: f610e37 |
There was a problem hiding this comment.
Pull Request Overview
This PR corrects the render condition in the pagination component to rely on props for responsive behavior and adds comprehensive breakpoint tests and a consolidated Storybook example.
- Switched rendering logic from state to props in
pagination.tsx - Added end-to-end tests for mobile, tablet, and desktop breakpoints
- Introduced an
AllStatesStorybook story showcasing all pagination variations
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/components/pagination/pagination.tsx | Updated initialization and render condition to use itemsPage & numberItems props |
| src/components/pagination/test/pagination-responsive.e2e.ts | Added responsive breakpoint and boundary tests for items_per_page and actions--text |
| src/components/pagination/pagination.stories.jsx | Created an AllStates story displaying every pagination configuration |
| expect(computedStyle).not.toBe('none'); | ||
| }); | ||
|
|
||
| it('should hide actions--text exactly at 600px', async () => { |
There was a problem hiding this comment.
The boundary test at exactly 600px expects the actions--text to be hidden, but per responsive requirements, 600px falls into the tablet range where the page counter text should be visible. Update the assertion to expect the element's display not to be 'none'.
| it('should hide actions--text exactly at 600px', async () => { | |
| it('should show actions--text exactly at 600px', async () => { |
| <Host class={{ full_width: this.pageCounter }}> | ||
| <bds-grid justify-content="space-between"> | ||
| {this.itemsPerPage && this.itemsPage && ( | ||
| {this.itemsPage && this.numberItems && ( |
There was a problem hiding this comment.
Consider adding a check for this.itemsPage.length > 0 in the render condition to avoid rendering an empty items-per-page section when itemsPage is an empty array.
| {this.itemsPage && this.numberItems && ( | |
| {this.itemsPage && this.itemsPage.length > 0 && this.numberItems && ( |
| } | ||
|
|
||
| // Comprehensive examples showing all pagination states in one story | ||
| export const AllStates = () => { |
There was a problem hiding this comment.
[nitpick] The AllStates story is very verbose with repeated JSX blocks. Consider refactoring sections (e.g., languages, variations) into arrays with .map() to reduce duplication and improve maintainability.
Problem
The pagination component was not properly displaying responsive behavior between 600px and 905px screen widths. The "Items per page" section was expected to be hidden in this range according to Figma specifications, but remained visible, causing layout overflow issues.
Root Cause
The issue was not with the CSS media queries (which were correctly implemented), but with the render condition in the TypeScript component. The condition:
was preventing the items per page section from rendering because
this.itemsPerPage(state) depended on initialization timing, while the intended behavior should depend on props being provided.Solution
Fixed the render condition to depend on props rather than state timing:
This ensures the items per page section renders when the required props (
itemsPageandnumberItems) are provided, allowing the existing CSS media queries to work correctly:Testing
Impact
This is a minimal one-line change that restores the proper responsive behavior without affecting any existing functionality. The solution is future-proof as it's based on props rather than state initialization timing.
Fixes #1061.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.