enhance: Introduce dataviews component and update webpack. #2497
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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 set of changes to the Dokan plugin, focusing on enhancing the REST API, implementing new React components, and introducing a sophisticated Tailwind CSS configuration. The modifications span across multiple areas including REST controllers, frontend routing, layout components, and styling infrastructure. Key additions include new abstract REST controllers, a vendor navigation menu checker, React dashboard components, and a scoped Tailwind CSS configuration that provides enhanced styling capabilities. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Router
participant Layout
participant ContentArea
participant DataViews
participant RESTController
Client->>Router: Request Dashboard
Router->>Layout: Render Layout
Layout->>ContentArea: Render Content
ContentArea->>DataViews: Initialize Table
DataViews->>RESTController: Fetch Data
RESTController-->>DataViews: Return Data
DataViews->>Layout: Render Table
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
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
CodeRabbit Configuration File (
|
MdAsifHossainNadim
changed the base branch from
develop
to
update/vendor-dashboard-structure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 7
🧹 Nitpick comments (47)
includes/Abstracts/DokanRESTBaseController.php (1)
35-66: Consider bounding the pagination size.
Theformat_collection_responsemethod is well-structured, but setting a maximum bound forper_pagecan guard against excessively large queries. Otherwise, this implementation aligns with standard WordPress REST patterns.includes/REST/DokanDataCountriesController.php (1)
85-98: Role-based access inget_items_permissions_check.
Granting access todokan_admin_menu_capability()and'dokandar'is appropriate for your plugin's roles. If you expand roles in the future, consider centralizing role checks to maintain clarity.includes/VendorNavMenuChecker.php (2)
69-78: Dependency resolution for routes.
is_dependency_clearedchecks overridden templates to conditionally adjust route usage. This is a neat approach to gracefully degrade or adjust menu links.
103-120: Resolving overridden templates.
get_template_dependencies_resolutionstransforms dependencies into actual template checks. Ensure the'dokan_get_dashboard_nav_template_dependency_resolutions'filter remains well-documented for plugin devs.includes/REST/CustomersController.php (3)
26-50: Clarify route documentation
While the docblock mentions the purpose of the new route, it would be beneficial to add more explicit documentation (e.g., expected parameters, examples) for the/searchroute. Clear docs help both internal developers and external consumers of this REST API.
52-83: Refine permission checks for better extensibility
Thecheck_permissionmethod currently maps a fixed set of actions (view, create, edit, delete, batch, search) to vendor permissions. Considering future expansions, using a filter or more granular role checks can make permission handling easier to extend or override.
280-285: Consider switching from filter toggling to using capabilities
perform_vendor_actiontemporarily adds and removeswoocommerce_rest_check_permissionsfilters. An alternative approach might be hooking in more granular capabilities or using dedicated Woocommerce permission callbacks. This can reduce side effects and keep permission logic in a single place.tests/php/src/REST/CustomersControllerTest.php (3)
35-59: Expand test data coverage
You've created multiple customers for testing, which is excellent. Consider adding a scenario with edge-case user data (e.g., empty last name, special characters in username) to more thoroughly test the controller’s resilience.
112-147: Add negative tests for get_items
There are tests for per_page and ordering, but also verify negative or borderline cases (e.g., invalid per_page, extremely large page number). This ensures the controller handles them gracefully.
278-330: Assert database changes after batch operations
The test verifies the response after create, update, and delete. Also consider verifying that changes are truly reflected in the database or user store for fully integrated testing.includes/Assets.php (3)
5-5: Validate the availability ofWCAdminAssets
Ensure thatAutomattic\WooCommerce\Internal\Admin\WCAdminAssetsis available in all supported WooCommerce versions. If older versions of WooCommerce are in use, consider fallback logic to avoid potential class-not-found issues.
536-597: Ensure script sizes remain reasonable
The new scripts (login-form-popup,sweetalert2,dokan-util-helper, etc.) may increase the admin bundle size. Consider lazy-loading them or conditionally enqueuing them only when needed to reduce overhead.
904-914: Safeguard admin script registration
Registering and then enqueuingdokan-react-frontendanddokan-react-componentsin the dashboard is excellent. As you leverageWCAdminAssets, double-check if these scripts are only loaded in contexts where they are actually used, preventing unnecessary load in the entire WP admin.src/Components/index.tsx (1)
1-2: Clarify usage
ExportingDataViewsanduseWindowDimensionsis helpful for reuse. If these exports are central to critical app workflows, add a short comment or descriptive doc to guide other developers on their usage.src/Layout/ContentArea.tsx (2)
1-3: Optimize imports
You are importingSlotfrom@wordpress/components, which is fine. If more WordPress components are anticipated, consider grouping them in a single import statement to keep code tidy.
4-15: Avoid unintentional re-renders
ContentAreareturns a fragment and includes<Sidebar>plus multiple<Slot>components. If performance becomes an issue, consider memoizing child components or using a layout approach that prevents unnecessary re-renders.includes/Abstracts/DokanRESTCustomerController.php (1)
19-19: Consider clarifying or translating$rest_basefurther in docs.
Although'customer'conveys the endpoint, you might want to specify in the docblock that this endpoint is primarily used for authenticated customer requests, distinguishing it from the vendor or admin endpoints.includes/Abstracts/DokanRESTVendorController.php (1)
19-19: Document the$rest_baseproperty more explicitly.
Although'vendor'is self-explanatory, adding a bit of contextual detail in the docblock about how this endpoint fits into the overall Dokan REST architecture could help maintainers.src/Layout/Header.tsx (2)
4-5: Add prop type definitions for maintainability.
Even a simple interface or type annotation clarifying thattitle?: stringhelps future contributors understand what props are expected here.-const Header = ( { title = '' } ) => { +import { FC } from 'react'; +interface HeaderProps { + title?: string; +} + +const Header: FC<HeaderProps> = ( { title = '' } ) => { const navigate = useNavigate();
8-18: Ensure placeholders or fallback for emptySlotusage.
If downstream components don’t supply fills for these slots, the layout might look inconsistent. Consider default placeholders or at least a graceful fallback.src/Dashboard/index.tsx (1)
11-12: Correct the variable name to "mappedRoutes".
Improve clarity and consistency by fixing the spelling. This helps future contributors understand the variable’s purpose at a glance.-const mapedRoutes = routes.map( ( route ) => { +const mappedRoutes = routes.map( ( route ) => {src/Hooks/ViewportDimensions.ts (2)
8-14: Add or update inline tags for clarity.
You might consider adding an explicit@returnsJSDoc tag or clarifying param usage for@since.
23-43: Ensure optimal performance or consider a debounce.
While usingrequestAnimationFramethrottles dimension updates, frequent resizing in some browsers can still lead to multiple calls per frame. If performance is a concern, you might consider a short debounce or further optimization.includes/REST/OrderControllerV3.php (1)
16-25: Versioning documentation.
Since this is a new version (v3), confirm that the relevant doc comments or changelog references the correct version to avoid confusion with older controllers.src/Layout/index.tsx (1)
1-4: Imports are logically grouped and descriptive.
Consider adding a short doc comment for each imported component to clarify usage if needed.src/Components/DataViews/DataViewTable.tsx (3)
1-6: Ensure consistent import ordering and naming.
You may want to keep the imports from external libraries (e.g.,@wordpress/dataviews,@wordpress/components) together, then local hooks, then type definitions, and finally styles. This improves readability and makes it easier for newcomers to navigate the file.
39-57: Add fallback or warning when no data is present.
Ifprops.datais empty, the table might render no rows. Consider showing a fallback message or a spinner ifisLoadingis true. This enhances the user experience by providing clarity.
59-59: Export as named export as well.
Consider adding a named export forDataViewTableto simplify imports in other parts of the codebase. This can be done alongside the default export, facilitating tree-shaking and more flexible importing patterns.+ export { DataViewTable }; export default DataViewTable;src/Routing/index.tsx (3)
1-7: Use consistent import ordering and identify potential code splitting.
This file imports both WordPress packages and React Router functionality. Evaluate whether any route definitions or large components can be code-split to reduce initial bundle size.
8-40: Avoid dynamic checking of valid elements in withRouter.
While cloning or creating an element is a valid approach, you might simplify the logic by ensuringComponentis always a function or class-based component. This approach can be more predictable and reduce potential type-safety issues.+ // Example approach: + // Make withRouter a higher-order function that returns a new functional component + // and pass the router props directly, rather than checking if `Component` is a valid element.
42-79: Leverage code splitting for NotFound or other large components.
IfNotFoundor other components might be less frequently used, you could dynamically import them to optimize performance for the main flow.base-tailwind.config.js (2)
6-7: Clarify usage of custom.dokan-layoutclass.
Ensure all relevant parent containers or root elements are wrapped with this class so that the styling is consistently applied and doesn’t leak outside.
8-94: Ensure critical styles remain unscoped if needed.
Some utilities, such as reset or normalizing styles, may need to remain unscoped for consistency across the entire page layout. Confirm that scoping them under.dokan-layoutdoes not cause visual discrepancies in other areas.webpack.config.js (1)
12-12: Name your new entry point meaningfully.
The key"frontend"is a bit generic. Consider more descriptive names, for instance"dokan-dashboard-frontend"or"dokan-frontend-react", to differentiate from other frontends.src/Layout/WPostsDataView.tsx (3)
177-177: Remove redundant double-negation to simplify condition.
The value fromview?.sort?.fieldis already coerced to boolean in a conditional. There is no need for!!.- if ( !! view?.sort?.field ) { + if (view?.sort?.field) {🧰 Tools
🪛 Biome (1.9.4)
[error] 177-177: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
182-182: Remove redundant double-negation to simplify condition.
Similar reasoning applies here; usingview?.sort?.directionon its own is sufficient.- if ( !! view?.sort?.direction ) { + if (view?.sort?.direction) {🧰 Tools
🪛 Biome (1.9.4)
[error] 182-182: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
187-187: Remove redundant double-negation to simplify condition.
Again, no need for!! view?.filters.- if ( !! view?.filters ) { + if (view?.filters) {🧰 Tools
🪛 Biome (1.9.4)
[error] 187-187: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
includes/REST/ProductController.php (1)
75-85: Check for potential performance overhead in meta queries.
Repeated meta queries (e.g.,_downloadable) can impact performance on large product sets. If this filter is widely used, consider adding an index or caching strategy.src/base-tailwind.scss (1)
6-21: Consider preserving minimal spacing for better readability.While resetting table styles is good for consistency, completely zeroing out all spacing might affect readability. Consider maintaining minimal padding for better content legibility.
@layer base { .dokan-layout { table, th, td { - margin: 0; - padding: 0; + margin: 0; + padding: 0.25rem 0.5rem; border: 0; border-spacing: 0; border-collapse: collapse;docs/feature-override/readme.md (3)
63-64: Fix React capitalization and improve clarity.Ensure consistent capitalization of "React" as it's a proper noun. Also, consider restructuring the sentences to improve readability.
-This will be used to determine if the menu is pointing to the react Application or to the Legacy PHP Route. +This will be used to determine if the menu is pointing to the React application or to the legacy PHP route. -The `route` property should be the same as the route defined in the `React` application in Router Array. +The `route` property should be the same as the route defined in the `React` application's router configuration.Also applies to: 66-69
🧰 Tools
🪛 LanguageTool
[grammar] ~63-~63: “React” is a proper noun and needs to be capitalized.
Context: ...h menu which we are indicating that the react route is available. This will be used ...(A_GOOGLE)
[grammar] ~64-~64: “React” is a proper noun and needs to be capitalized.
Context: ...etermine if the menu is pointing to the react Application or to the Legacy PHP Route....(A_GOOGLE)
91-92: Improve clarity of migration documentation.The explanation about when to define override templates is a bit convoluted. Consider breaking it into clearer steps.
-If you are writing a new feature or modifying an existing feature in the `React` application, you do not need to define the override templates for the `React` route. -But if you are modifying or migrating an existing feature written in PHP to the `React` application and you want that if some of the PHP template is overridden by the existing PHP template then the legacy PHP page will be displayed, then you need to define the override templates for the `React` route. +Override templates are only needed when: +1. You are migrating an existing PHP feature to React +2. The existing PHP feature has template overrides +3. You want to maintain compatibility with these overridden templates + +For new React features, you do not need to define override templates.🧰 Tools
🪛 LanguageTool
[uncategorized] ~92-~92: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ritten in PHP to theReactapplication and you want that if some of the PHP templa...(COMMA_COMPOUND_SENTENCE)
136-145: Add type information to the template structure documentation.The array structure documentation would benefit from type information for better clarity.
[ - 'route_name' => [ + 'route_name' => [ // string [ - 'slug' => 'template-slug', - 'name' => 'template-name' // (Optional), - 'args' = [] // (Optional) + 'slug' => 'template-slug', // string (required) + 'name' => 'template-name', // string (optional) + 'args' => [] // array (optional) ], ] ]docs/api/api.md (2)
5-5: Fix typo in anchor linkThere's a typo in the anchor link "role-specifix-controllers" which should be "role-specific-controllers".
-## 1. Role-Specifix Controllers. +## 1. Role-Specific Controllers🧰 Tools
🪛 Markdownlint (0.37.0)
5-5: null
Link fragments should be valid(MD051, link-fragments)
96-98: Add error handling to check_permission methodThe permission check should handle potential errors when the user capability check fails.
Consider adding error handling:
public function check_permission() { if ( ! current_user_can( 'dokandar' ) ) { return new WP_Error( 'dokan_rest_cannot_view', __( 'Sorry, you are not allowed to view this resource.', 'dokan' ), [ 'status' => rest_authorization_required_code() ] ); } return true; }docs/frontend/dataviews.md (3)
27-27: Fix grammatical error in component descriptionThe sentence has a subject-verb agreement error.
-Also, an example component are designed to interact with WordPress REST APIs +Also, an example component is designed to interact with WordPress REST APIs🧰 Tools
🪛 LanguageTool
[grammar] ~27-~27: “component” is a singular noun. It appears that the verb form is incorrect.
Context: ...r dashboard. Also, an example component are designed to interact with WordPress RES...(PCT_SINGULAR_NOUN_PLURAL_VERB_AGREEMENT)
39-65: Add error handling for asset registrationThe script registration example should include error handling for missing assets.
Consider adding error handling:
$script_assets = 'add your script assets path here'; if ( ! file_exists( $script_assets ) ) { error_log( sprintf( 'Error: Asset file %s does not exist', $script_assets ) ); return; } try { $vendor_asset = require $script_assets; // ... rest of the registration code } catch ( Exception $e ) { error_log( sprintf( 'Error loading asset file: %s', $e->getMessage() ) ); return; }
708-708: Remove duplicate text in callback descriptionThe callback description contains duplicate text.
-When we need to click & redirect action to another page, we can use this property. When we need to click \& redirect action to another page, we can use this property. +When we need to click & redirect action to another page, we can use this property.
🛑 Comments failed to post (7)
includes/Abstracts/DokanRESTCustomerController.php (1)
28-30: 🛠️ Refactor suggestion
Validate user’s role in addition to checking login status.
Whileis_user_logged_in()ensures authentication, you may also consider confirming the user’s role matches “customer.” This helps safeguard against unauthorized usage by other roles who are merely logged in.public function check_permission() { - return is_user_logged_in(); + return is_user_logged_in() && current_user_can( 'customer_capability' ); }Committable suggestion skipped: line range outside the PR's diff.
src/Dashboard/index.tsx (1)
38-42: 🛠️ Refactor suggestion
Handle missing
.dashboard-content-areagracefully.
Using the non-null assertion!is convenient, but you risk runtime errors if the element isn’t present. A fallback or error message could improve resilience.const rootElement = document.querySelector( '.dashboard-content-area' ); -if ( rootElement ) { - const root = createRoot( rootElement ); - root.render( <App /> ); -} +if (!rootElement) { + console.error('Root element not found for the dashboard'); + return; +} +const root = createRoot(rootElement); +root.render(<App />);📝 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.domReady( function () { const rootElement = document.querySelector( '.dashboard-content-area' ); if (!rootElement) { console.error('Root element not found for the dashboard'); return; } const root = createRoot(rootElement); root.render( <App /> ); } );src/Components/DataViews/DataViewTable.tsx (1)
35-37:
⚠️ Potential issueGuard against missing hooks object.
wp.hooks.applyFiltersimplies the globalwpobject is available. Consider gracefully handling the situation ifwporwp.hooksis undefined, to avoid a runtime error in non-WordPress environments.+ if (typeof wp?.hooks?.applyFilters !== 'function') { + return element; + }Committable suggestion skipped: line range outside the PR's diff.
webpack.config.js (1)
76-80:
⚠️ Potential issueCheck for library collisions.
Defining a globalwindowlibrary named['dokan', '[name]']can cause collisions if multiple bundles share the same name. Ensure name uniqueness, especially if the plugin can be active in parallel with other code that uses the same global space.docs/api/api.md (2)
21-23: 🛠️ Refactor suggestion
Add implementation details for format_collection_response
The
format_collection_responsemethod implementation details are missing. This is a crucial method that handles pagination and response formatting.Consider adding the implementation details:
public function format_collection_response( $response, $request, $total_items ) { $total_pages = ceil( $total_items / (int) $request['per_page'] ); $response->header( 'X-WP-Total', (int) $total_items ); $response->header( 'X-WP-TotalPages', (int) $total_pages ); return $response; }
260-260:
⚠️ Potential issueFix syntax error in permission check
There's a duplicate semicolon in the permission check condition.
-return is_user_logged_in() || dokan_is_seller_enabled( get_current_user_id() );; +return is_user_logged_in() || dokan_is_seller_enabled( get_current_user_id() );📝 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.return is_user_logged_in() || dokan_is_seller_enabled( get_current_user_id() );docs/frontend/dataviews.md (1)
669-671:
⚠️ Potential issueFix incorrect slot name in after-table slot
The after-table slot is using the same name as the before-table slot.
-<Slot name={ `dokan-before-vendor-data-table-${ props?.namespace }` } fillProps={{ ...filteredProps }} /> +<Slot name={ `dokan-after-vendor-data-table-${ props?.namespace }` } fillProps={{ ...filteredProps }} />📝 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.{/* After dokan data table rendered slot */} <Slot name={ `dokan-after-vendor-data-table-${ props?.namespace }` } fillProps={{ ...filteredProps }} /> </div>
MdAsifHossainNadim
added
the
Needs: Dev Review
mrabbani
added
Needs: Author Reply
and removed
Needs: Dev Review
…andle responsive preview from base components.
…ce-dataviews-from-dokan-free
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
#4047
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
Bug Fixes
Performance
Development