feat(pagination): add new pagination component
#106
Conversation
…ular into pagination # Conflicts: # libs/flowbite-angular/core/flowbite.theme.init.ts # libs/flowbite-angular/pagination/index.ts # libs/flowbite-angular/pagination/pagination.component.ts # libs/flowbite-angular/pagination/pagination.theme.service.ts # libs/flowbite-angular/pagination/pagination.theme.ts
# Conflicts: # libs/flowbite-angular/core/flowbite.theme.init.ts # libs/flowbite-angular/pagination/index.ts # libs/flowbite-angular/pagination/pagination.component.ts # libs/flowbite-angular/pagination/pagination.theme.service.ts # libs/flowbite-angular/pagination/pagination.theme.ts
…ular into pagination # Conflicts: # libs/flowbite-angular/core/flowbite.theme.init.ts
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces a comprehensive pagination component to the Flowbite Angular library. The implementation includes a new secondary entry point for pagination, with extensive customization options, theming capabilities, and documentation. The changes span multiple files across documentation, component implementation, and configuration, adding robust pagination functionality with support for various navigation styles, custom theming, and flexible page management. Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (20)
libs/flowbite-angular/pagination/pagination.component.ts (1)
27-36: Consider allowing consumers to override the default pagination style more granularly.
Currently, the entire pagination theme custom style is treated as a single object. It may be beneficial to allow partial overrides at the level of specific pagination elements (e.g., first/last buttons, page tabs), so consumers can fine-tune each element.apps/docs/docs/components/pagination/_both.component.html (1)
1-4: Ensure support for customizable labels in 'both' mode.
The component currently uses a fixed “First/Last” or “Previous/Next” text alongside icons. If this is intended to support multiple languages or dynamic text, add input properties, e.g., “previousLabel,” “nextLabel,” etc.apps/docs/docs/components/pagination/_text.component.html (1)
1-4: Add dynamic data binding for the initial page or total items.
Currently, both “currentPage” and “totalItems” are hard-coded. If you plan to demonstrate changing pagination states in this example, consider binding them to component properties (e.g., [currentPage]="myPageVar") for clarity and future extensibility.apps/docs/docs/components/pagination/_both.component.ts (1)
5-10: Consider adding component documentation and type safetyWhile the component structure is correct, consider these improvements:
- Add JSDoc comments to document the component's purpose and usage
- Consider implementing
OnInitif initialization logic will be needed- Add strict typing by enabling
strict: truein your component metadata+/** + * Demonstrates pagination component with both text and icon navigation + */ @Component({ selector: 'flowbite-demo-pagination-both', imports: [PaginationComponent], templateUrl: './_both.component.html', + standalone: true, }) -export class FlowbiteBothComponent {} +export class FlowbiteBothComponent implements OnInit { + ngOnInit(): void { + // Add initialization logic here + } +}apps/docs/docs/components/pagination/_custom.component.ts (1)
5-10: Consider creating a base demo componentGiven the similar structure across demo components, consider:
- Creating a base demo component or abstract class
- Adding component documentation
- Including example code in the component itself
+/** + * Demonstrates pagination component with custom styling + * @example + * <flowbite-demo-pagination-custom> + * <!-- Add example usage here --> + * </flowbite-demo-pagination-custom> + */ @Component({ selector: 'flowbite-demo-pagination-custom', imports: [PaginationComponent], templateUrl: './_custom.component.html', + standalone: true, }) -export class FlowbiteCustomComponent {} +export class FlowbiteCustomComponent extends BasePaginationDemo { + // Implement custom styling logic +}apps/docs/docs/components/pagination/_custom.component.html (1)
5-5: Document thefirstLastproperty behavior.Consider adding a comment explaining that
[firstLast]="false"hides the first/last page buttons, as this might not be immediately obvious to users.apps/docs/docs/components/pagination/ng-doc.page.ts (1)
9-20: Enhance documentation metadata.Consider adding a description and tags to improve documentation searchability and organization.
const pagination: NgDocPage = { title: 'Pagination', mdFile: './index.md', category: ComponentCategory, order: 10, + description: 'Pagination component for navigating through multiple pages of content', + tags: ['navigation', 'pagination', 'components'], demos: { flowbiteDefaultComponent: FlowbiteDefaultComponent, flowbiteTextComponent: FlowbiteTextComponent, flowbiteBothComponent: FlowbiteBothComponent, flowbiteCustomComponent: FlowbiteCustomComponent, }, };libs/flowbite-angular/pagination/index.ts (1)
1-1: Overall PR AssessmentThe pagination component implementation is progressing well, but several items from the PR objectives need attention:
- Icons for navigation buttons are still pending
- Internationalization support needs to be implemented
- Size variants should be added as suggested above
The code structure and organization are solid, with good attention to theming and customization. Consider addressing the suggested improvements before finalizing the PR.
libs/flowbite-angular/pagination/pagination.theme.ts (2)
20-22: Fix incorrect JSDoc commentThe JSDoc comment references
NavbarComponentinstead ofPaginationComponent.- * Theme definition for `NavbarComponent` + * Theme definition for `PaginationComponent`
35-42: Consider adding theme properties for different sizesThe PR objectives mention implementing "different size variables for the pagination component". The current theme structure doesn't include size-related properties.
Consider extending the theme to include size variants:
export interface PaginationTheme { root: { base: string; sizes: { sm: string; md: string; lg: string; }; }; // ... existing properties }libs/flowbite-angular/pagination/pagination.theme.service.ts (1)
27-36: LGTM! Consider adding input validationThe implementation correctly merges themes and processes classes. However, consider adding validation for the properties parameter.
public getClasses(properties: PaginationProperties): PaginationClass { + if (!properties?.customStyle) { + return { + rootClass: twMerge(this.baseTheme.root.base), + navigationClass: twMerge(this.baseTheme.navigation.base), + }; + } const theme: PaginationTheme = mergeTheme(this.baseTheme, properties.customStyle); // ... rest of the implementationapps/docs/docs/components/pagination/index.md (1)
29-29: Fix grammatical issue in section titleChange "Pagination with text and icon" to "Pagination with text and icons" for correct grammatical number.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...ript" ``` ## Pagination with text and icon {{ NgDocActions.demo('flowbiteBothComp...(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
libs/flowbite-angular/pagination/pagination-button.theme.service.ts (1)
33-41: Consider adding type safety for custom stylesThe
mergeThemeoperation could benefit from additional type checking for custom styles.public getClasses(properties: PaginationButtonProperties): PaginationButtonClass { + if (!properties?.customStyle) { + properties = { ...properties, customStyle: {} }; + } const theme: PaginationButtonTheme = mergeTheme(this.baseTheme, properties.customStyle); const output: PaginationButtonClass = { rootClass: twMerge(theme.root.base, theme.root.active[properties.active]), }; return output; }libs/flowbite-angular/pagination/pagination-button.theme.ts (2)
27-28: Consider adding hover state handling for disabled buttonsThe base class includes hover states, but there's no specific handling for disabled buttons which could lead to inconsistent UX.
base: 'flex items-center justify-center px-3 h-8 ms-0 leading-tight text-gray-500 bg-white border border-e-0 border-gray-300 hover:bg-gray-100 hover:text-gray-700 dark:bg-gray-800 dark:border-gray-700 dark:text-gray-400 dark:hover:bg-gray-700 dark:hover:text-white first:rounded-l-lg last:rounded-r-lg', + disabled: 'cursor-not-allowed opacity-50 hover:bg-white hover:text-gray-500 dark:hover:bg-gray-800 dark:hover:text-gray-400',
15-20: Add size variants to theme interfaceAs mentioned in the PR objectives, size variants are planned. Consider adding the interface structure now.
export interface PaginationButtonTheme { root: { base: string; active: FlowbiteBoolean; + size: { + small: string; + default: string; + large: string; + }; }; }apps/docs/docs/ng-doc.api.ts (1)
Line range hint
1-1: Consider implementing remaining features using separate servicesBased on the PR objectives, several features are still pending. Consider the following architectural approaches:
For icon customization:
- Create an
IconConfigServiceto manage icon configurations- Use dependency injection to allow icon overrides
For i18n support:
- Implement an abstract
PaginationI18nService- Use Angular's i18n infrastructure or a translation library
For size variants:
- Add size enum and interface
- Implement size-specific styles in theme
Would you like assistance in implementing any of these features?
libs/flowbite-angular/pagination/pagination-button.directive.ts (2)
32-35: Enhance documentation for themeServiceThe documentation comment could be more descriptive about the service's purpose and usage.
- /** - * Service injected used to generate class - */ + /** + * Theme service responsible for generating CSS classes based on button state and custom styles. + * Injected automatically through DI. + */
27-31: Consider i18n preparation for button labelsSince internationalization support is in the PR's pending tasks, consider adding an input for customizable button labels that can be translated:
// Future enhancement suggestion: @Input() ariaLabel?: string; @Input() buttonText?: string;This would support the pending task of "adding the ability to customize button labels and support for internationalization" mentioned in the PR objectives.
apps/docs/docs/components/pagination/_default.component.ts (2)
21-23: Enhance test data generationThe current implementation uses repetitive data. Consider adding more realistic and varied test data.
- this._testDatas.push({ id: i, name: 'John Doe' }); + this._testDatas.push({ + id: i, + name: `User ${i}`, + });
4-4: Add missing testsThe PR objectives indicate that tests haven't been specified yet.
Would you like me to help generate unit tests for this component? The tests should cover:
- Page change handling
- Edge cases (first/last page)
- Error scenarios
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/docs/docs/components/pagination/_both.component.html(1 hunks)apps/docs/docs/components/pagination/_both.component.ts(1 hunks)apps/docs/docs/components/pagination/_custom.component.html(1 hunks)apps/docs/docs/components/pagination/_custom.component.ts(1 hunks)apps/docs/docs/components/pagination/_default.component.html(1 hunks)apps/docs/docs/components/pagination/_default.component.ts(1 hunks)apps/docs/docs/components/pagination/_text.component.html(1 hunks)apps/docs/docs/components/pagination/_text.component.ts(1 hunks)apps/docs/docs/components/pagination/index.md(1 hunks)apps/docs/docs/components/pagination/ng-doc.page.ts(1 hunks)apps/docs/docs/ng-doc.api.ts(1 hunks)libs/flowbite-angular/core/flowbite.theme.init.ts(6 hunks)libs/flowbite-angular/pagination/README.md(1 hunks)libs/flowbite-angular/pagination/index.ts(1 hunks)libs/flowbite-angular/pagination/ng-package.json(1 hunks)libs/flowbite-angular/pagination/pagination-button.directive.ts(1 hunks)libs/flowbite-angular/pagination/pagination-button.theme.service.ts(1 hunks)libs/flowbite-angular/pagination/pagination-button.theme.ts(1 hunks)libs/flowbite-angular/pagination/pagination.component.ts(1 hunks)libs/flowbite-angular/pagination/pagination.theme.service.ts(1 hunks)libs/flowbite-angular/pagination/pagination.theme.ts(1 hunks)tsconfig.base.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- libs/flowbite-angular/pagination/README.md
- libs/flowbite-angular/pagination/ng-package.json
🧰 Additional context used
🪛 Biome (1.9.4)
apps/docs/docs/components/pagination/_default.component.ts
[error] 17-17: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🪛 LanguageTool
apps/docs/docs/components/pagination/index.md
[uncategorized] ~29-~29: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...ript" ``` ## Pagination with text and icon {{ NgDocActions.demo('flowbiteBothComp...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
🔇 Additional comments (8)
apps/docs/docs/components/pagination/_default.component.html (1)
2-2: Verify data access pattern
The testDatas().items access pattern suggests direct function calls in the template. Consider using a signal or observable pattern instead for better performance.
apps/docs/docs/components/pagination/_custom.component.html (1)
12-12: Verify color contrast ratios for accessibility.
The custom colors used (neutral-400, neutral-700) should be verified for sufficient contrast ratios to ensure WCAG compliance.
apps/docs/docs/ng-doc.api.ts (1)
35-35: LGTM: Documentation entry properly added
The pagination component is correctly added to the API documentation scope.
tsconfig.base.json (1)
35-35: LGTM! Path mapping follows project conventions
The pagination module path mapping is correctly added and follows the established pattern for Flowbite Angular components.
libs/flowbite-angular/pagination/pagination-button.directive.ts (1)
27-54: Well-structured directive implementation!
The directive follows Angular best practices:
- Standalone directive with clear selector
- Proper use of signals with input() and model()
- Clean inheritance from BaseComponent
- Type-safe theme implementation
libs/flowbite-angular/core/flowbite.theme.init.ts (3)
121-130: LGTM: Pagination imports follow established patterns
The new pagination-related imports are well-organized and include all necessary components for theme integration.
484-485: LGTM: Default value providers are properly integrated
The pagination default value providers are correctly added and follow the established pattern.
121-130: Verify pagination module integration
Let's ensure that all required pagination-related files are present and properly structured.
✅ Verification successful
Let me verify the exports to ensure they match the imports in the theme init file.
Pagination module integration is properly structured and exported
The verification confirms that:
- All required pagination files are present with consistent naming
- The imports in
flowbite.theme.init.tsmatch the exports from the pagination module - All theme-related tokens, services, and configurations are properly exported through the module's index.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify pagination module structure and exports
# Check if pagination module directory exists and has required files
echo "Checking pagination module structure..."
fd -t f . -e ts --base-directory libs/flowbite-angular/pagination
# Verify exports of pagination components
echo -e "\nChecking pagination exports..."
rg -l "export.*pagination" libs/flowbite-angular/pagination
# Verify consistent naming patterns
echo -e "\nChecking naming consistency..."
rg -l "(pagination|Pagination)" libs/flowbite-angular/pagination
Length of output: 1337
Script:
#!/bin/bash
# Check the exports in index.ts and specific files to verify they match the imports
echo "Checking index.ts exports..."
cat libs/flowbite-angular/pagination/index.ts
echo -e "\nChecking theme-related exports..."
rg "export.*FLOWBITE_PAGINATION.*_THEME_TOKEN|export.*pagination.*Theme|export.*PaginationThemeService" libs/flowbite-angular/pagination/pagination.theme.ts libs/flowbite-angular/pagination/pagination-button.theme.ts -A 1
Length of output: 1685
| @Component({ | ||
| selector: 'flowbite-pagination', | ||
| standalone: true, | ||
| imports: [PaginationButtonDirective, IconComponent], | ||
| template: `<nav | ||
| [class]="contentClasses().navigationClass" | ||
| [ariaLabel]="ariaLabel()"> | ||
| @if (firstLast()) { | ||
| <button | ||
| type="button" | ||
| (click)="firstPage()" | ||
| [customStyle]="buttonCustomStyle()" | ||
| flowbitePaginationButton> | ||
| @switch (navigation()) { | ||
| @case ('icon') { | ||
| <flowbite-icon | ||
| svgIcon="flowbite-angular:chevron-right" | ||
| class="w-5 h-5 rotate-180" /> | ||
| <flowbite-icon | ||
| svgIcon="flowbite-angular:chevron-right" | ||
| class="w-5 h-5 rotate-180" /> | ||
| } | ||
| @case ('text') { | ||
| <span>First</span> | ||
| } | ||
| @case ('both') { | ||
| <flowbite-icon | ||
| svgIcon="flowbite-angular:chevron-right" | ||
| class="w-5 h-5 rotate-180" /> | ||
| <flowbite-icon | ||
| svgIcon="flowbite-angular:chevron-right" | ||
| class="w-5 h-5 rotate-180" /> | ||
| <span>First</span> | ||
| } | ||
| } | ||
| </button> | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add i18n/ARIA best practices for the 'First' button text.
The template supports textual labels for the “First” button, but there's no clear mechanism to customize them for different locales. Consider adding an input property, like “firstLabel,” with a default of “First,” so that the label can be easily localized.
| @if (firstLast()) { | ||
| <button | ||
| type="button" | ||
| (click)="lastPage()" | ||
| [customStyle]="buttonCustomStyle()" | ||
| flowbitePaginationButton> | ||
| @switch (navigation()) { | ||
| @case ('icon') { | ||
| <flowbite-icon | ||
| svgIcon="flowbite-angular:chevron-right" | ||
| class="w-5 h-5" /> | ||
| <flowbite-icon | ||
| svgIcon="flowbite-angular:chevron-right" | ||
| class="w-5 h-5" /> | ||
| } | ||
| @case ('text') { | ||
| <span>Last</span> | ||
| } | ||
| @case ('both') { | ||
| <span>Last</span> | ||
| <flowbite-icon | ||
| svgIcon="flowbite-angular:chevron-right" | ||
| class="w-5 h-5" /> | ||
| <flowbite-icon | ||
| svgIcon="flowbite-angular:chevron-right" | ||
| class="w-5 h-5" /> | ||
| } | ||
| } | ||
| </button> | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add i18n/ARIA best practices for the 'Last' button text.
Similarly, you could introduce an input property (e.g., “lastLabel”) so the “Last” link text can be localized.
| return this.currentPage() - Math.floor(this.tabs() / 2); | ||
| }); | ||
|
|
||
| /** | ||
| * Value of the maximum pages calculated from `totalItems` | ||
| */ | ||
| readonly maxPages = computed(() => { | ||
| return Math.max(Math.ceil(this.totalItems() / this.pageSize()), 1); | ||
| }); | ||
|
|
||
| /** | ||
| * Array of the visible page tabs | ||
| */ | ||
| readonly visiblePages = computed(() => { | ||
| const pages: number[] = []; | ||
| const visibleTabs = Math.min(this.tabs(), this.maxPages()); | ||
|
|
||
| for (let i = this.firstPageToShow(); i < this.firstPageToShow() + visibleTabs; i++) { | ||
| pages.push(i); | ||
| } | ||
|
|
||
| return pages; | ||
| }); | ||
|
|
||
| /** | ||
| * Real value of the current page | ||
| * | ||
| * If the given `currentPage` is bigger than the `maxPages`, | ||
| * the last page will be the active one | ||
| */ | ||
| readonly visibleCurrentPage = computed(() => { | ||
| return Math.min(this.currentPage(), this.maxPages()); | ||
| }); | ||
|
|
||
| /** | ||
| * Set the custom style for this pagination | ||
| */ | ||
| public customStyle = model(inject(FLOWBITE_PAGINATION_CUSTOM_STYLE_DEFAULT_VALUE)); | ||
| /** | ||
| * Set the custom style for this pagination's button | ||
| */ | ||
| public buttonCustomStyle = model(inject(FLOWBITE_PAGINATION_BUTTON_CUSTOM_STYLE_DEFAULT_VALUE)); | ||
|
|
||
| //#region BaseComponent implementation | ||
| public override fetchClass(): PaginationClass { | ||
| return this.themeService.getClasses({ | ||
| customStyle: this.customStyle(), | ||
| }); | ||
| } | ||
|
|
||
| public override init(): void { | ||
| this.iconRegistry.addRawSvgIconInNamepsace( | ||
| 'flowbite-angular', | ||
| 'chevron-right', | ||
| this.domSanitizer.bypassSecurityTrustHtml(CHEVRON_RIGHT_SVG_ICON) | ||
| ); | ||
| } | ||
| //#endregion | ||
|
|
||
| /** | ||
| * Sets the value of the `currentPage` | ||
| * @param page number of the active page | ||
| */ | ||
| changePage(page: number) { | ||
| if (this.visibleCurrentPage() === page) return; | ||
| this.currentPage.set(page); | ||
| } | ||
|
|
||
| /** | ||
| * Decreases the value of `currentPage` if it's bigger than 1 | ||
| */ | ||
| previousPage() { | ||
| if (this.visibleCurrentPage() === 1) return; | ||
| this.currentPage.update((value) => value - 1); | ||
| } | ||
|
|
||
| /** | ||
| * Increases the value of `currentPage` if it's smaller than `maxPages` | ||
| */ | ||
| nextPage() { | ||
| if (this.visibleCurrentPage() === this.maxPages()) return; | ||
| this.currentPage.update((value) => value + 1); | ||
| } | ||
|
|
||
| /** | ||
| * Sets the value of `currentPage` to 1 | ||
| */ | ||
| firstPage() { | ||
| if (this.currentPage() === 1) return; | ||
| this.currentPage.set(1); | ||
| } | ||
|
|
||
| /** | ||
| * Sets the value of `currentPage` equal to `maxPages` | ||
| */ | ||
| lastPage() { | ||
| if (this.currentPage() === this.maxPages()) return; | ||
| this.currentPage.set(this.maxPages()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Leverage the “pageChange” output event.
“pageChange” is declared (line 234) but never emitted within the component’s logic. If you intend for external consumers to respond to page changes, you could emit an event each time the page value updates, for example, right after the call to “this.currentPage.set(page).”
Here's one potential approach:
--- a/pagination.component.ts
+++ b/pagination.component.ts
@@ -312,6 +312,7 @@ changePage(page: number) {
if (this.visibleCurrentPage() === page) return;
this.currentPage.set(page);
+ this.pageChange.emit(page);
}Committable suggestion skipped: line range outside the PR's diff.
| import { PaginationComponent } from 'flowbite-angular/pagination'; | ||
|
|
||
| import { Component } from '@angular/core'; | ||
|
|
||
| @Component({ | ||
| selector: 'flowbite-demo-pagination-text', | ||
| imports: [PaginationComponent], | ||
| templateUrl: './_text.component.html', | ||
| }) | ||
| export class FlowbiteTextComponent {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a dedicated property and event emitter for page changes.
This example references the pagination component but does not display how to handle page changes. To make the documentation more instructive, you might add a property like “page: number = 1” and subscribe to the (pageChange) event for demonstration.
| <table> | ||
| @for (item of testDatas().items; track $index) { | ||
| <tr> | ||
| <td>{{ item.id }}</td> | ||
| <td>{{ item.name }}</td> | ||
| </tr> | ||
| } | ||
| </table> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes and table headers
The table structure needs improvement for accessibility:
- Missing table headers
- Missing accessibility attributes
- No loading or error states handled
-<table>
+<table role="grid" aria-label="Pagination demo data">
+ <thead>
+ <tr>
+ <th scope="col">ID</th>
+ <th scope="col">Name</th>
+ </tr>
+ </thead>
+ <tbody>
@for (item of testDatas().items; track $index) {
<tr>
- <td>{{ item.id }}</td>
- <td>{{ item.name }}</td>
+ <td role="gridcell">{{ item.id }}</td>
+ <td role="gridcell">{{ item.name }}</td>
</tr>
}
+ </tbody>
</table>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <table> | |
| @for (item of testDatas().items; track $index) { | |
| <tr> | |
| <td>{{ item.id }}</td> | |
| <td>{{ item.name }}</td> | |
| </tr> | |
| } | |
| </table> | |
| <table role="grid" aria-label="Pagination demo data"> | |
| <thead> | |
| <tr> | |
| <th scope="col">ID</th> | |
| <th scope="col">Name</th> | |
| </tr> | |
| </thead> | |
| <tbody> | |
| @for (item of testDatas().items; track $index) { | |
| <tr> | |
| <td role="gridcell">{{ item.id }}</td> | |
| <td role="gridcell">{{ item.name }}</td> | |
| </tr> | |
| } | |
| </tbody> | |
| </table> |
|
|
||
| readonly currentPage = signal(1); | ||
| readonly pageSize = signal(10); | ||
| readonly testDatas = signal(PagedResult.empty<IdNameDto>()); |
There was a problem hiding this comment.
Fix variable declaration order
The static analysis tool correctly flags that PagedResult is used before its declaration.
Move the PagedResult interface and constant declarations above the component class.
🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
|
|
||
| @Component({ | ||
| // ng-doc-ignore-line | ||
| selector: 'flowbite-demo-navbar-default', |
There was a problem hiding this comment.
Incorrect selector name
The selector 'flowbite-demo-navbar-default' doesn't match the component's pagination functionality.
- selector: 'flowbite-demo-navbar-default',
+ selector: 'flowbite-demo-pagination-default',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| selector: 'flowbite-demo-navbar-default', | |
| selector: 'flowbite-demo-pagination-default', |
| pageChangeHandler(page: number) { | ||
| this.currentPage.set(page); | ||
|
|
||
| // Call your API here | ||
| // The following lines of code simulates a backend query | ||
| const _items: IdNameDto[] = []; | ||
| const _start = (this.currentPage() - 1) * this.pageSize(); | ||
| const _end = Math.min(this._testDatas.length, _start + this.pageSize()); | ||
|
|
||
| for (let i = _start; i < _end; i++) { | ||
| _items.push({ id: i + 1, name: 'John Doe' }); | ||
| } | ||
|
|
||
| this.testDatas.set({ | ||
| items: _items, | ||
| totalItems: this._testDatas.length, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety and error handling to pageChangeHandler
The page change handler could benefit from additional type safety and error handling.
- pageChangeHandler(page: number) {
+ pageChangeHandler(page: number): void {
+ if (page < 1) {
+ console.warn('Invalid page number');
+ return;
+ }
+
this.currentPage.set(page);
- // Call your API here
- // The following lines of code simulates a backend query
- const _items: IdNameDto[] = [];
+ // Simulate API call with proper error handling
+ try {
+ const _items: IdNameDto[] = [];
const _start = (this.currentPage() - 1) * this.pageSize();
const _end = Math.min(this._testDatas.length, _start + this.pageSize());
for (let i = _start; i < _end; i++) {
- _items.push({ id: i + 1, name: 'John Doe' });
+ _items.push(this._testDatas[i]);
}
this.testDatas.set({
items: _items,
totalItems: this._testDatas.length,
});
+ } catch (error) {
+ console.error('Error updating pagination:', error);
+ this.testDatas.set(PagedResult.empty());
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| { | ||
| provide: PaginationThemeService, | ||
| useClass: PaginationThemeService, | ||
| }, |
There was a problem hiding this comment.
Remove duplicate PaginationThemeService registration
The PaginationThemeService is registered twice in the service providers array. This could lead to unexpected behavior in the dependency injection system.
Apply this change to remove the duplicate registration:
{
provide: PaginationThemeService,
useClass: PaginationThemeService,
},
{
provide: NavbarContentThemeService,
useClass: NavbarContentThemeService,
},
// ... other services ...
- {
- provide: PaginationThemeService,
- useClass: PaginationThemeService,
- },
{
provide: PaginationButtonThemeService,
useClass: PaginationButtonThemeService,
},Also applies to: 280-287
| { | ||
| provide: FLOWBITE_PAGINATION_THEME_TOKEN, | ||
| useValue: paginationTheme, | ||
| }, |
There was a problem hiding this comment.
Remove duplicate pagination theme token registration
The FLOWBITE_PAGINATION_THEME_TOKEN is registered twice in the theme providers array. This could lead to unexpected behavior in the theme system.
Apply this change to remove the duplicate registration:
{
provide: FLOWBITE_PAGINATION_THEME_TOKEN,
useValue: paginationTheme,
},
{
provide: FLOWBITE_NAVBAR_ITEM_THEME_TOKEN,
useValue: navbarItemTheme,
},
// ... other themes ...
- {
- provide: FLOWBITE_PAGINATION_THEME_TOKEN,
- useValue: paginationTheme,
- },
{
provide: FLOWBITE_PAGINATION_BUTTON_THEME_TOKEN,
useValue: paginationButtonTheme,
},Also applies to: 420-427
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
libs/flowbite-angular/pagination/pagination.component.ts (2)
88-88: 🛠️ Refactor suggestionAdd i18n support for navigation button labels.
The hardcoded English strings "First" and "Last" should be externalized for internationalization.
Consider adding input properties for these labels:
+ readonly firstLabel = input('First'); + readonly lastLabel = input('Last');Then update the template:
- <span>First</span> + <span>{{ firstLabel() }}</span>- <span>Last</span> + <span>{{ lastLabel() }}</span>Also applies to: 211-211
403-406: 🛠️ Refactor suggestionAdd pageChange event emitter.
The
changePagemethod updates the page but doesn't notify parent components of the change.Add an event emitter and emit the new page value:
+ readonly pageChange = output<number>(); changePage(page: number) { if (this.visibleCurrentPage() === page) return; this.currentPage.set(page); + this.pageChange.emit(page); }
🧹 Nitpick comments (8)
libs/flowbite-angular/pagination/pagination.theme.ts (2)
29-30: Fix incorrect component reference in documentation.The comment references
NavbarComponentinstead ofPaginationComponent.- * Theme definition for `NavbarComponent` + * Theme definition for `PaginationComponent`
16-18: Consider using a more specific type for size keys.The string index signature allows any string key, which could lead to runtime errors. Consider using a union type of allowed values instead.
-export interface PaginationSizes extends Pick<FlowbiteSizes, 'sm' | 'md'> { - [key: string]: string; -} +export interface PaginationSizes extends Pick<FlowbiteSizes, 'sm' | 'md'> { + // Add specific additional sizes if needed + // lg?: string; + // xl?: string; +}libs/flowbite-angular/pagination/pagination-button.theme.ts (2)
31-31: Consider splitting the base styles for better readability.The long string of CSS classes could be split into logical groups (layout, colors, states) for better maintainability.
- base: 'flex items-center justify-center px-3 h-8 ms-0 leading-tight text-gray-500 bg-white border border-e-0 border-gray-300 hover:bg-gray-100 hover:text-gray-700 dark:bg-gray-800 dark:border-gray-700 dark:text-gray-400 dark:hover:bg-gray-700 dark:hover:text-white first:rounded-l-lg last:rounded-r-lg', + base: [ + // Layout + 'flex items-center justify-center px-3 h-8 ms-0 leading-tight', + // Borders + 'border border-e-0 border-gray-300 first:rounded-l-lg last:rounded-r-lg', + // Colors + 'text-gray-500 bg-white hover:bg-gray-100 hover:text-gray-700', + // Dark mode + 'dark:bg-gray-800 dark:border-gray-700 dark:text-gray-400 dark:hover:bg-gray-700 dark:hover:text-white' + ].join(' '),
32-36: Consider extracting color values to theme variables.The hardcoded color values could be moved to theme variables for better customization support.
Consider implementing a color token system that allows for easy customization of the color scheme while maintaining consistency across components.
libs/flowbite-angular/pagination/pagination.component.ts (1)
384-396: Add cleanup for registered icons.The component registers SVG icons but doesn't clean them up when the component is destroyed.
Consider adding cleanup in the destroy hook:
+ override destroy(): void { + this.iconRegistry.removeIconInNamespace('flowbite-angular', 'chevron-right'); + this.iconRegistry.removeIconInNamespace('flowbite-angular', 'chevron-double-right'); + super.destroy(); + }libs/flowbite-angular/pagination/pagination-button.directive.ts (3)
17-26: Add JSDoc comments for the injection token and providerConsider adding comprehensive documentation for:
- The structure and expected properties of
PaginationButtonTheme- Usage examples for custom style overrides
- Purpose and use cases of the default value provider
+/** + * Injection token for customizing the default pagination button styles. + * @example + * ```typescript + * { + * base: 'custom-base-class', + * active: 'custom-active-class', + * // ... other theme properties + * } + * ``` + */ export const FLOWBITE_PAGINATION_BUTTON_CUSTOM_STYLE_DEFAULT_VALUE = new InjectionToken< DeepPartial<PaginationButtonTheme> >('FLOWBITE_PAGINATION_BUTTON_CUSTOM_STYLE_DEFAULT_VALUE'); +/** + * Default provider for pagination button styles. + * Use this provider to set up default custom styles at the application level. + */ export const paginationButtonDefaultValueProvider = makeEnvironmentProviders([
47-47: Add size validationConsider adding validation for the size input to ensure only valid sizes are used.
- public readonly size = input.required<keyof PaginationSizes>(); + public readonly size = input.required<keyof PaginationSizes>({ + transform: (value: keyof PaginationSizes) => { + const validSizes: (keyof PaginationSizes)[] = ['sm', 'md', 'lg']; + return validSizes.includes(value) ? value : 'md'; + } + });
54-60: Add unit tests for the directiveThe PR objectives mention missing tests. Please add comprehensive unit tests for:
- Class generation with different sizes
- Active state handling
- Custom style application
Would you like me to help generate the unit test suite for this directive?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
libs/flowbite-angular/pagination/pagination-button.directive.ts(1 hunks)libs/flowbite-angular/pagination/pagination-button.theme.service.ts(1 hunks)libs/flowbite-angular/pagination/pagination-button.theme.ts(1 hunks)libs/flowbite-angular/pagination/pagination.component.ts(1 hunks)libs/flowbite-angular/pagination/pagination.theme.service.ts(1 hunks)libs/flowbite-angular/pagination/pagination.theme.ts(1 hunks)libs/flowbite-angular/utils/icon.list.ts(1 hunks)libs/flowbite-angular/utils/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/flowbite-angular/pagination/pagination-button.theme.service.ts
- libs/flowbite-angular/pagination/pagination.theme.service.ts
🔇 Additional comments (5)
libs/flowbite-angular/utils/index.ts (1)
12-12: LGTM! Verify icon usage in pagination components.
The export is correctly placed and follows the established naming convention.
Let's verify the icon usage in pagination components:
✅ Verification successful
Icon is correctly used in pagination component
The verification confirms that CHEVRON_DOUBLE_RIGHT_SVG_ICON is:
- Defined in
utils/icon.list.ts - Exported through
utils/index.ts - Properly imported and used in
pagination.component.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CHEVRON_DOUBLE_RIGHT_SVG_ICON usage in pagination components
rg "CHEVRON_DOUBLE_RIGHT_SVG_ICON" --type ts
Length of output: 491
libs/flowbite-angular/pagination/pagination.theme.ts (1)
47-64: LGTM! Well-structured theme definition.
The theme structure is clean and follows Flowbite's design system with appropriate dark mode support.
libs/flowbite-angular/pagination/pagination-button.theme.ts (1)
1-47: LGTM! Well-structured theme implementation.
The overall structure is clean and follows Angular best practices. The theme implementation provides good flexibility for customization while maintaining default styles.
libs/flowbite-angular/utils/icon.list.ts (1)
26-30: LGTM! SVG icon implementation is consistent.
The new CHEVRON_DOUBLE_RIGHT_SVG_ICON follows the established pattern and best practices:
- Uses
currentColorfor theming compatibility - Maintains consistent SVG attributes
- Properly structured path data
libs/flowbite-angular/pagination/pagination-button.directive.ts (1)
5-6: Verify potential circular dependency
Importing from 'flowbite-angular' within a library that's part of the same package could create circular dependencies.
| <button | ||
| type="button" | ||
| (click)="firstPage()" | ||
| [customStyle]="buttonCustomStyle()" | ||
| [size]="size()" | ||
| flowbitePaginationButton> | ||
| @switch (navigation()) { | ||
| @case ('icon') { | ||
| @if (lastIcon()) { | ||
| <div class="rotate-180"> | ||
| <ng-container [ngTemplateOutlet]="lastIcon()!" /> | ||
| </div> | ||
| } @else { | ||
| <flowbite-icon | ||
| svgIcon="flowbite-angular:chevron-double-right" | ||
| [class]="contentClasses().iconClass" | ||
| class="rotate-180" /> | ||
| } | ||
| } | ||
| @case ('text') { | ||
| <span>First</span> | ||
| } | ||
| @case ('both') { | ||
| @if (lastIcon()) { | ||
| <div class="rotate-180"> | ||
| <ng-container [ngTemplateOutlet]="lastIcon()!" /> | ||
| </div> | ||
| } @else { | ||
| <flowbite-icon | ||
| svgIcon="flowbite-angular:chevron-double-right" | ||
| [class]="contentClasses().iconClass" | ||
| class="rotate-180" /> | ||
| } | ||
| <span>First</span> | ||
| } | ||
| } | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for navigation buttons.
Navigation buttons should have appropriate ARIA attributes to improve screen reader support.
Add these attributes to the first/last buttons:
<button
type="button"
(click)="firstPage()"
+ [attr.aria-label]="firstLabel()"
+ [attr.aria-disabled]="currentPage() === 1"
[customStyle]="buttonCustomStyle()"
[size]="size()"
flowbitePaginationButton> <button
type="button"
(click)="lastPage()"
+ [attr.aria-label]="lastLabel()"
+ [attr.aria-disabled]="currentPage() === maxPages()"
[customStyle]="buttonCustomStyle()"
[size]="size()"
flowbitePaginationButton>Also applies to: 194-225
| previousPage() { | ||
| if (this.visibleCurrentPage() === 1) return; | ||
| this.currentPage.update((value) => value - 1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unit tests for navigation methods.
The navigation methods need test coverage to ensure they handle edge cases correctly.
Consider adding these test cases:
- Previous page at first page
- Next page at last page
- Navigation within bounds
- Page size changes
- Total items changes
Also applies to: 419-422
| @Directive({ | ||
| standalone: true, | ||
| selector: 'button[flowbitePaginationButton]', | ||
| }) | ||
| export class PaginationButtonDirective extends BaseComponent<PaginationButtonClass> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA attributes for accessibility
The pagination button should include appropriate ARIA attributes for better accessibility.
@Directive({
standalone: true,
selector: 'button[flowbitePaginationButton]',
+ host: {
+ 'role': 'button',
+ '[attr.aria-current]': 'active() ? "page" : null',
+ '[attr.aria-label]': '"Page " + label',
+ }
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Directive({ | |
| standalone: true, | |
| selector: 'button[flowbitePaginationButton]', | |
| }) | |
| export class PaginationButtonDirective extends BaseComponent<PaginationButtonClass> { | |
| @Directive({ | |
| standalone: true, | |
| selector: 'button[flowbitePaginationButton]', | |
| host: { | |
| 'role': 'button', | |
| '[attr.aria-current]': 'active() ? "page" : null', | |
| '[attr.aria-label]': '"Page " + label', | |
| } | |
| }) | |
| export class PaginationButtonDirective extends BaseComponent<PaginationButtonClass> { |
ReviewRefacto
Inputs
Template
[...]
template: `
<...>
`
[...]
@if (['icon', 'both'].includes(navigation())){}
@if (['text', 'both'].includes(navigation())){}
Buttons
<...>
<flowbite-button
[size]="properties.size"
[color]="properties.color"
...>
<...>
</flowbite-button>
<...>(where To achieve this behavior, we can use an input type ButtonProperties public buttonProperties = model<DeepPartial<ButtonProperties>>({});(Imagine with an With this solution, we are not forced to use a Directive and let the user choose the style of its button Additional informationOn the internationalization part, for the moment we are not adding anything for it. Let's use English only and see if users want to have some flexibility on this. Also, I don't think that flowbite-react for example has something related to internationalization either so... |
feat(pagination): remove directive & use flowbite-button
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
libs/flowbite-angular/pagination/pagination.theme.ts (2)
4-11: Consider using an enum or union type for navigation typesThe
PaginationNavigationinterface could benefit from stronger type safety. Instead of using string properties, consider using an enum or union type to enforce specific navigation types, especially since the PR mentions pending icon fixes.+export type NavigationType = 'icon' | 'text' | 'both'; + export interface PaginationNavigation { - icon: string; - text: string; - both: string; + type: NavigationType; + content: string; }
13-18: Consider constraining size optionsThe string index signature allows any additional sizes, which could lead to inconsistencies. Since the PR mentions implementing specific size variables, consider:
- Explicitly defining all supported sizes
- Documenting the expected size values
- Adding validation for custom sizes
-export interface PaginationSizes extends Pick<FlowbiteSizes, 'sm' | 'md'> { - [key: string]: string; +export interface PaginationSizes extends Pick<FlowbiteSizes, 'sm' | 'md' | 'lg'> { + // Custom sizes should follow the pattern: 'text-{size}' + [key: `text-${string}`]: string; }apps/docs/docs/components/pagination/ng-doc.page.ts (2)
9-13: Enhance the component documentation.The current JSDoc documentation could be more comprehensive. Consider adding:
- Available props and their types
- Usage examples
- Accessibility considerations
- Browser compatibility notes
/** * Use the pagination component to show a list of buttons to navigate in your tables + * + * @example + * ```html + * <flowbite-pagination + * [currentPage]="1" + * [totalPages]="10" + * (pageChange)="onPageChange($event)"> + * </flowbite-pagination> + * ``` + * + * @props + * - currentPage: number - The current active page + * - totalPages: number - Total number of pages + * - size: 'sm' | 'md' | 'lg' - Size of the pagination component + * + * @events + * - pageChange: EventEmitter<number> - Emitted when page changes + * + * @accessibility + * - Follows WCAG 2.1 guidelines for navigation + * - Supports keyboard navigation * * @status:info NEW */
14-25: Consider reorganizing demos for better documentation flow.The current demo structure could be reorganized to better guide users through the component's features:
- Basic usage (already covered by
flowbiteDefaultComponent)- Sizes and variants
- Navigation styles
- Customization examples
- Internationalization examples
Also, consider adding a description for each demo in the configuration:
demos: { - flowbiteDefaultComponent: FlowbiteDefaultComponent, + flowbiteDefaultComponent: { + component: FlowbiteDefaultComponent, + description: 'Basic pagination with numbers only', + order: 1 + }, // Add similar descriptions for other demos },libs/flowbite-angular/pagination/pagination.component.ts (1)
363-373: Mark internal computed properties as protected.The following computed properties should be marked as protected since they are implementation details:
firstPageToShowmaxPagesvisiblePagesvisibleCurrentPagevisiblePagesCountAlso applies to: 378-380, 385-397, 405-407, 412-414
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/docs/docs/components/pagination/_custom.component.html(1 hunks)apps/docs/docs/components/pagination/ng-doc.page.ts(1 hunks)apps/docs/docs/getting-started/quickstart/ng-doc.page.ts(1 hunks)apps/docs/docs/getting-started/versioning/index.md(1 hunks)apps/docs/docs/getting-started/versioning/ng-doc.page.ts(1 hunks)libs/flowbite-angular/core/flowbite.theme.init.ts(4 hunks)libs/flowbite-angular/pagination/index.ts(1 hunks)libs/flowbite-angular/pagination/pagination.component.ts(1 hunks)libs/flowbite-angular/pagination/pagination.theme.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/docs/docs/getting-started/quickstart/ng-doc.page.ts
- apps/docs/docs/getting-started/versioning/ng-doc.page.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/docs/docs/components/pagination/_custom.component.html
- libs/flowbite-angular/core/flowbite.theme.init.ts
- libs/flowbite-angular/pagination/index.ts
🔇 Additional comments (9)
apps/docs/docs/getting-started/versioning/index.md (1)
9-10: Please clarify the versioning strategy.
A few concerns regarding the version table updates:
-
Two new versions (1.3.0 and 1.4.0) are being added simultaneously. Since this PR is marked as WIP, should we be documenting these versions before the feature is complete?
-
The TailwindCSS version requirement has been relaxed from
^3.0.24to^3.0.0. Please verify if this change is intentional and won't introduce compatibility issues.
Run the following script to check for potential TailwindCSS compatibility issues:
✅ Verification successful
Let me gather more information about the versioning strategy by checking the version history and package dependencies.
Version changes appear to be correctly documented
Based on the verification results:
-
The TailwindCSS version change from
^3.0.24to^3.0.0is intentional and safe:- The project currently uses TailwindCSS 3.4.17 in development
- The relaxed requirement
^3.0.0still ensures compatibility with breaking changes (major version) - This aligns with semantic versioning best practices
-
The version bumps are appropriate:
- Version 1.3.0 matches the current package version in
libs/flowbite-angular/package.json - Version 1.4.0 is likely planned for the pagination feature, which is actively being developed (evidenced by multiple pagination-related files)
- Both versions maintaining the same dependency requirements ensures consistency
- Version 1.3.0 matches the current package version in
The versioning strategy appears sound, documenting both the current and upcoming versions with appropriate compatibility requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in TailwindCSS versions
# Check package.json for current TailwindCSS version
echo "Current TailwindCSS version requirement:"
fd package.json --exec grep -A 5 "tailwindcss"
# Check for any TailwindCSS imports or configurations
echo -e "\nTailwindCSS usage in codebase:"
rg -l "tailwindcss" --type ts --type js --type json
Length of output: 885
Script:
#!/bin/bash
# Check version history in the versioning doc
echo "Version history in docs:"
cat apps/docs/docs/getting-started/versioning/index.md
# Check package.json versions
echo -e "\nFlowbite Angular package version:"
cat libs/flowbite-angular/package.json | grep -A 2 '"version"'
# Check for recent changes related to pagination
echo -e "\nPagination-related files:"
rg -l "pagination" --type ts --type html
Length of output: 1823
libs/flowbite-angular/pagination/pagination.theme.ts (2)
47-64: LGTM! Well-structured theme implementation
The theme implementation:
- Follows Flowbite's theming patterns
- Includes dark mode support
- Provides appropriate size variations
- Uses semantic Tailwind classes
66-72: LGTM! Clean class interface definition
The interface properly extends FlowbiteClass and includes all necessary class properties for the pagination component.
apps/docs/docs/components/pagination/ng-doc.page.ts (1)
19-24: Verify the demo component implementations.
The demo components are properly mapped, but let's ensure they cover all the planned features mentioned in the PR objectives.
✅ Verification successful
Demo components provide good coverage of pagination features
The demo components effectively showcase the key pagination features:
FlowbiteDefaultComponent: Demonstrates practical pagination with data handling and page size controlFlowbiteTextComponent: Shows text-only navigation modeFlowbiteBothComponent: Illustrates combined text and icon navigationFlowbiteCustomComponent: Exhibits customization capabilities including:- Custom page size
- ARIA label support
- Style customization
- First/Last navigation control
The components provide a comprehensive demonstration of the pagination functionality, covering both basic usage and advanced customization options.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if demo components implement all planned features
# Check for icon customization in demo components
echo "Checking for icon customization implementations..."
rg -l "ChevronLeft|ChevronRight|DoubleChevronLeft|DoubleChevronRight" apps/docs/docs/components/pagination/_*.component.ts
# Check for size variants
echo "Checking for size variant implementations..."
rg -l "\[size\]=" apps/docs/docs/components/pagination/_*.component.ts
# Check for i18n support
echo "Checking for i18n implementations..."
rg -l "i18n" apps/docs/docs/components/pagination/_*.component.ts
Length of output: 1084
Script:
#!/bin/bash
# Let's check the actual content of these demo components to verify their implementations
echo "=== Default Component ==="
cat apps/docs/docs/components/pagination/_default.component.ts
echo -e "\n=== Text Component ==="
cat apps/docs/docs/components/pagination/_text.component.ts
echo -e "\n=== Both Component ==="
cat apps/docs/docs/components/pagination/_both.component.ts
echo -e "\n=== Custom Component ==="
cat apps/docs/docs/components/pagination/_custom.component.ts
Length of output: 2883
Script:
#!/bin/bash
# Let's check the template files to see the actual usage of the pagination component
echo "=== Default Component Template ==="
cat apps/docs/docs/components/pagination/_default.component.html
echo -e "\n=== Text Component Template ==="
cat apps/docs/docs/components/pagination/_text.component.html
echo -e "\n=== Both Component Template ==="
cat apps/docs/docs/components/pagination/_both.component.html
echo -e "\n=== Custom Component Template ==="
cat apps/docs/docs/components/pagination/_custom.component.html
Length of output: 1291
libs/flowbite-angular/pagination/pagination.component.ts (5)
1-127: LGTM! Well-structured dependency injection setup.
The injection tokens and their default providers are well-organized, type-safe, and follow consistent naming patterns.
157-157: Add i18n support for navigation text.
The navigation text is hardcoded in English. Consider using translation keys for internationalization.
Also applies to: 182-182, 219-219, 243-243
134-136: Enhance accessibility attributes.
Add more ARIA attributes to improve screen reader support:
- Add
aria-current="page"for the current page - Add
aria-disabledfor disabled navigation buttons - Add descriptive
aria-labelfor page numbers
Also applies to: 188-198
443-446: Emit pageChange event when page updates.
The pageChange event should be emitted when the page value changes to notify parent components.
451-454: Add unit tests for navigation methods.
The navigation methods need test coverage to ensure they handle edge cases correctly.
Also applies to: 459-462, 467-470, 475-478
| /** | ||
| * Theme definition for `NavbarComponent` | ||
| */ |
There was a problem hiding this comment.
Fix incorrect JSDoc reference
The JSDoc comment incorrectly references NavbarComponent instead of PaginationComponent.
/**
- * Theme definition for `NavbarComponent`
+ * Theme definition for `PaginationComponent`
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Theme definition for `NavbarComponent` | |
| */ | |
| /** | |
| * Theme definition for `PaginationComponent` | |
| */ |
| /** | ||
| * Required properties for class generation of `PaginationComponent` | ||
| */ | ||
| export interface PaginationProperties { | ||
| customStyle: DeepPartial<PaginationTheme>; | ||
| size: keyof PaginationSizes; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add i18n support for button labels
Since the PR objectives mention "adding the ability to customize button labels and support for internationalization", consider extending PaginationProperties to include i18n-related properties:
export interface PaginationProperties {
customStyle: DeepPartial<PaginationTheme>;
size: keyof PaginationSizes;
+ labels?: {
+ first?: string;
+ previous?: string;
+ next?: string;
+ last?: string;
+ };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Required properties for class generation of `PaginationComponent` | |
| */ | |
| export interface PaginationProperties { | |
| customStyle: DeepPartial<PaginationTheme>; | |
| size: keyof PaginationSizes; | |
| } | |
| /** | |
| * Required properties for class generation of `PaginationComponent` | |
| */ | |
| export interface PaginationProperties { | |
| customStyle: DeepPartial<PaginationTheme>; | |
| size: keyof PaginationSizes; | |
| labels?: { | |
| first?: string; | |
| previous?: string; | |
| next?: string; | |
| last?: string; | |
| }; | |
| } |
| <flowbite-icon | ||
| svgIcon="flowbite-angular:chevron-double-right" | ||
| [class]="contentClasses().iconClass" | ||
| class="rotate-180" /> | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace rotated icons with dedicated left-facing icons.
Instead of using rotate-180 class on right-facing icons, use dedicated left-facing icons (chevron-left and chevron-double-left) for better maintainability and performance.
-<flowbite-icon
- svgIcon="flowbite-angular:chevron-double-right"
- [class]="contentClasses().iconClass"
- class="rotate-180" />
+<flowbite-icon
+ svgIcon="flowbite-angular:chevron-double-left"
+ [class]="contentClasses().iconClass" />Also applies to: 175-179
| public override init(): void { | ||
| this.iconRegistry.addRawSvgIconInNamepsace( | ||
| 'flowbite-angular', | ||
| 'chevron-right', | ||
| this.domSanitizer.bypassSecurityTrustHtml(CHEVRON_RIGHT_SVG_ICON) | ||
| ); | ||
|
|
||
| this.iconRegistry.addRawSvgIconInNamepsace( | ||
| 'flowbite-angular', | ||
| 'chevron-double-right', | ||
| this.domSanitizer.bypassSecurityTrustHtml(CHEVRON_DOUBLE_RIGHT_SVG_ICON) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Register left-facing icons in init method.
Add registration for chevron-left and chevron-double-left icons to support the refactoring of rotated icons.
public override init(): void {
+ this.iconRegistry.addRawSvgIconInNamepsace(
+ 'flowbite-angular',
+ 'chevron-left',
+ this.domSanitizer.bypassSecurityTrustHtml(CHEVRON_LEFT_SVG_ICON)
+ );
+
+ this.iconRegistry.addRawSvgIconInNamepsace(
+ 'flowbite-angular',
+ 'chevron-double-left',
+ this.domSanitizer.bypassSecurityTrustHtml(CHEVRON_DOUBLE_LEFT_SVG_ICON)
+ );
+
this.iconRegistry.addRawSvgIconInNamepsace(
'flowbite-angular',
'chevron-right',
this.domSanitizer.bypassSecurityTrustHtml(CHEVRON_RIGHT_SVG_ICON)
);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
libs/flowbite-angular/utils/icon.list.ts (1)
38-42: Consider extracting common SVG attributes to reduce duplication.While the implementation is correct, consider creating a shared interface or type for common SVG attributes to improve maintainability. This would help ensure consistency across all icons and make future updates easier.
Example approach:
interface IconSvgAttributes { xmlns: string; fill: string; viewBox: string; } const DEFAULT_SVG_ATTRIBUTES: IconSvgAttributes = { xmlns: 'http://www.w3.org/2000/svg', fill: 'none', viewBox: '0 0 24 24' }; const createIconSvg = (path: string): string => { const attrs = DEFAULT_SVG_ATTRIBUTES; return ` <svg xmlns="${attrs.xmlns}" fill="${attrs.fill}" viewBox="${attrs.viewBox}"> ${path} </svg> `; };libs/flowbite-angular/pagination/pagination.theme.service.ts (1)
27-36: Consider adding fallback logic for unknown or invalid size properties.
If an invalidproperties.sizekey is passed,twMergemight fail to apply classes or generate unexpected styling. A fallback (e.g.,'md') helps avoid runtime errors and ensures consistent behavior.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/docs/docs/components/pagination/_custom.component.html(1 hunks)libs/flowbite-angular/pagination/index.ts(1 hunks)libs/flowbite-angular/pagination/pagination.component.ts(1 hunks)libs/flowbite-angular/pagination/pagination.theme.service.ts(1 hunks)libs/flowbite-angular/pagination/pagination.theme.ts(1 hunks)libs/flowbite-angular/utils/icon.list.ts(1 hunks)libs/flowbite-angular/utils/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/flowbite-angular/utils/index.ts
- apps/docs/docs/components/pagination/_custom.component.html
- libs/flowbite-angular/pagination/index.ts
🔇 Additional comments (9)
libs/flowbite-angular/pagination/pagination.component.ts (2)
34-81: LGTM! Consistent naming convention for injection tokens.
The injection tokens follow a consistent naming pattern and are well-documented with appropriate types.
153-174: 🛠️ Refactor suggestion
Add accessibility attributes to the First button.
The First button lacks proper ARIA attributes for accessibility. Add aria-label and aria-disabled attributes.
<flowbite-button
type="button"
data-active="false"
+ [attr.aria-label]="'Go to first page'"
+ [attr.aria-disabled]="currentPage() === 1"
(click)="goToFirstPage()"
[color]="buttonProperties().color!"
[fill]="buttonProperties().fill!"Likely invalid or redundant comment.
libs/flowbite-angular/utils/icon.list.ts (3)
20-24: LGTM! Left chevron icon implementation is consistent.
The SVG implementation follows the established pattern and maintains consistency with other icons in the file.
32-36: Double chevron icon addresses the first/last button requirement.
The implementation is consistent with other icons and provides the necessary visual indicator for first/last page navigation.
20-42: Verify icon integration with the pagination component.
The implementation provides all necessary icons for pagination (previous/next and first/last). However, ensure these are properly integrated with the pagination component's navigation functions mentioned in the PR comments (goToFirst, etc.).
✅ Verification successful
Let me gather more specific information about the pagination component implementation.
Icons are properly integrated with pagination navigation
The verification confirms that:
- All chevron icons (single and double) are correctly imported and registered in the pagination component
- The icons are properly connected to their respective navigation functions:
CHEVRON_DOUBLE_LEFT_SVG_ICONwithgoToFirstPage()CHEVRON_LEFT_SVG_ICONwithgoToPreviousPage()CHEVRON_RIGHT_SVG_ICONwithgoToNextPage()CHEVRON_DOUBLE_RIGHT_SVG_ICONwithgoToLastPage()
- Each navigation function includes appropriate boundary checks and page updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of these icons in the pagination component
# Search for icon usage in pagination component
rg -l "CHEVRON_.*_SVG_ICON" --type ts
# Check if the icons are used with the correct navigation functions
ast-grep --pattern 'class $_ {
$$$
goTo$_($$$) {
$$$
}
$$$
}'
Length of output: 545
Script:
#!/bin/bash
# Check the pagination component implementation
rg -A 5 "CHEVRON_.*_SVG_ICON" libs/flowbite-angular/pagination/pagination.component.ts
# Check for navigation method implementations
rg -A 3 "goTo(First|Last|Previous|Next)" libs/flowbite-angular/pagination/pagination.component.ts
Length of output: 2220
libs/flowbite-angular/pagination/pagination.theme.service.ts (1)
1-6: No issues found with imports and InjectionToken usage.
All imports and token definitions are well-structured and clear. The naming convention for FLOWBITE_PAGINATION_THEME_TOKEN follows the recommended underscore style.
libs/flowbite-angular/pagination/pagination.theme.ts (3)
20-26: Add i18n support for button labels
This was previously requested. It would align with the PR objectives and user needs for easy localization and customization (e.g., “previous”, “next” in multiple languages).
28-30: Correct the JSDoc reference
The comment references NavbarComponent instead of PaginationComponent. Update it accordingly to avoid confusion.
44-58: Theme definition looks good
The default theme configuration for the pagination component is well-structured, providing clear base classes and size variations. This ensures users can easily customize pagination styles.
MGREMY
changed the title
pagination component [wip]pagination component
refactor(pagination): update post update post comment
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
libs/flowbite-angular/pagination/pagination.component.ts (5)
151-152: Consider removing redundant type castingThe
as DeepPartial<ButtonTheme>andas DeepPartial<ButtonProperties>type assertions are redundant since TypeScript can infer these types from the injection token's type definition.- } as DeepPartial<ButtonTheme>, + }, - } as DeepPartial<ButtonProperties>, + },
212-213: Enhance accessibility for page buttonsAdd
aria-current="page"for the active page button to improve screen reader support.[attr.data-active]="page === currentPage()" +[attr.aria-current]="page === currentPage() ? 'page' : undefined" (click)="goToPage(page)"
466-468: Improve error message specificityThe error message could be more helpful by indicating the expected values and their purpose.
- throw new Error('Either `totalPages` or `totalItems` must have a value'); + throw new Error('Pagination requires either `totalPages` (number of pages) or `totalItems` (total number of items) to be set');
430-430: Add validation for negative valuesThe
maxPagescomputation should handle negativetotalItemsvalues.- return Math.max(Math.ceil(this.totalItems()! / this.pageSize()), 1); + return Math.max(Math.ceil(Math.max(this.totalItems()!, 0) / Math.max(this.pageSize(), 1)), 1);
496-519: Enhance method documentationConsider adding more descriptive JSDoc comments for navigation methods, including:
- Return type
- Example usage
- Edge case behavior
Example for
goToPage:/** * Sets the value of the `currentPage` if it's between 1 and `maxPages` * @param page - Number of the target page * @returns void * @example * // Go to page 5 * pagination.goToPage(5); * * // No effect if page is out of bounds * pagination.goToPage(0); // No effect * pagination.goToPage(maxPages + 1); // No effect */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/flowbite-angular/pagination/pagination.component.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
libs/flowbite-angular/pagination/pagination.component.ts (1)
Learnt from: MGREMY
PR: themesberg/flowbite-angular#106
File: libs/flowbite-angular/pagination/pagination.component.ts:480-483
Timestamp: 2024-12-30T23:08:54.395Z
Learning: In Angular with the signals-based model approach (ModelInput), a separate EventEmitter for output is not required because the model() automatically provides an output for data updates.
🔇 Additional comments (1)
libs/flowbite-angular/pagination/pagination.component.ts (1)
458-494: LGTM! Base component implementation is solid
The implementation properly handles initialization and class fetching.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Issue Number
Issue Number: N/A
Does this PR introduce a breaking change?
Other information
Remaining works to do:
possibility to customize buttons' label + internalizationlibs/flowbite-angular/core/flowbite.theme.init.tsPaginationButtonDirective(big thanks @MGREMY )Summary by CodeRabbit
Summary by CodeRabbit
Release Notes: Flowbite Angular Pagination Component
New Features
Documentation
Improvements