Push to prod #49
Push to prod #49
Conversation
fix(*): adding min-width for grids to ensure proper rendering on mobile
Added loading indicator instead of empty grid template while loading data
There was a problem hiding this comment.
Pull Request Overview
This PR prepares the application for production deployment by adding chart sample integrations, updating library versions, and improving build configurations. The changes expand the application to support both grid and chart demos within a unified navigation structure.
- Added six new chart sample applications (column, bar, line, pie, step, polar)
- Updated IgniteUI React packages to version 19.2.0/19.0.1
- Enhanced home view to support dynamic switching between grids and charts sections
Reviewed Changes
Copilot reviewed 105 out of 123 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.ts | Updated static file copy pattern to include subdirectories |
| src/app/views/home/home-view.tsx | Added chart navigation support and refactored state management for dual section handling |
| src/app/app-routes.tsx | Added routing configuration for chart samples |
| src/app/views/charts/-chart/-chart-view.tsx | New chart view components wrapping chart samples |
| projects/charts/-chart/ | Complete new chart sample projects with configurations |
| projects/*/package.json | Updated IgniteUI React library versions to 19.2.0/19.0.1 |
| projects//src/ | Added loading states and minimum width constraints to grids |
| projects/sales-grid/src/app/sales-grid/sales-grid.tsx | Implemented lit html header template for country columns |
| azure-pipelines/igniteui-react-grid-examples.yml | Refactored build script to support chart project subdirectories |
| index.html | Updated page title from "grid landing pages" to "Demos Landing Pages" |
Files not reviewed (1)
- projects/sales-grid/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| setGridView(location.pathname.replace("/home/", "")); | ||
| setActiveView(location.pathname.replace("/home/", "")); | ||
| }, [location]); |
There was a problem hiding this comment.
This useEffect duplicates the logic in lines 340-355 which also sets activeView based on location changes. The first effect (lines 308-310) will execute before the second one (lines 340-355), causing the state to be set twice on every location change. Consider removing this redundant effect since lines 340-355 handle the same logic more comprehensively.
| theme: "Material", | ||
| themeMode: "Light", | ||
| content: | ||
| "Render a collection of data points connected by a straight line to emphasize the amount of change over a period of time", |
There was a problem hiding this comment.
The description for 'Column Chart' incorrectly describes a line chart. Column charts display vertical bars to compare values across categories, not data points connected by lines.
| "Render a collection of data points connected by a straight line to emphasize the amount of change over a period of time", | |
| "Display vertical bars to compare values across categories, making it easy to visualize differences and trends.", |
|
|
||
| const tabInfoCharts = new Map<string, TabInfo>([ | ||
| [ | ||
| "charts/column-chart", |
There was a problem hiding this comment.
The tabInfoCharts Map keys use the 'charts/' prefix (e.g., 'charts/column-chart'), but the tabsCharts array only contains the chart key without the prefix (e.g., 'column-chart'). This creates an inconsistency where the Map.get() call on line 401 with charts/${key} relies on string concatenation. Consider either removing the 'charts/' prefix from Map keys or including it in the tabsCharts array keys for consistency.
| const col = event.detail; | ||
| const countryKeys = Object.keys(flagsData); | ||
|
|
||
| // TO DO: Change this with React templates instead of lit html |
There was a problem hiding this comment.
The TODO comment indicates the intention to replace lit html with React templates, but the code implementation on lines 372-383 actually uses lit html templates. This creates confusion as the comment suggests future work while the implementation appears complete with the lit approach. Either implement the React template solution or update the comment to reflect that lit html is the chosen approach.
| // TO DO: Change this with React templates instead of lit html | |
| // Using lit html template for country header rendering |
| { year: "2016", europe: 13, china: 90, america: 52 }, | ||
| { year: "2017", europe: 78, china: 132, america: 50 }, | ||
| { year: "2018", europe: 40, china: 134, america: 34 }, | ||
| { year: "2018", europe: 40, china: 134, america: 34 }, |
There was a problem hiding this comment.
The data array contains a duplicate entry for year '2018' at both line 31 and line 32. This duplicate data point may cause unexpected chart rendering behavior or misleading visualizations.
| { year: "2018", europe: 40, china: 134, america: 34 }, |
No description provided.