Skip to content

Conversation

johnnyhuy
Copy link
Contributor

@johnnyhuy johnnyhuy commented Jun 16, 2024

PR Type

enhancement


Description

  • Adds new icon components for Grafana, Prometheus, and AWS with SVG content.
  • Adjusts Grid item spacing and padding in EntityPage.tsx for a consistent layout.
  • Updates route path for ScaffolderPage to "/self-service".
  • Integrates CssBaseline with the UnifiedThemeProvider for global style normalization in App.tsx.
  • Adds a new YAML file for defining a Backstage group in the examples directory.

Changes walkthrough 📝

Relevant files
Enhancement
Grafana.tsx
Add Grafana Icon Component                                                             

packages/app/src/components/icons/Grafana.tsx

  • Adds a new Grafana icon component with SVG content.
+43/-0   
Prometheus.tsx
Add Prometheus Icon Component                                                       

packages/app/src/components/icons/Prometheus.tsx

  • Adds a new Prometheus icon component with SVG content.
+43/-0   
Aws.tsx
Add AWS Icon Component                                                                     

packages/app/src/components/icons/Aws.tsx

  • Adds a new AWS icon component with SVG content.
+29/-0   
EntityPage.tsx
Adjust EntityPage Layout and Spacing                                         

packages/app/src/components/catalog/EntityPage.tsx

  • Adjusts Grid item spacing and padding across various EntityPage
    components for consistent layout.
  • Removes negative margin from Grid items.
  • +50/-50 
    App.tsx
    Update Route Paths and Integrate CssBaseline with Theme   

    packages/app/src/App.tsx

  • Updates route path to "/self-service" for the ScaffolderPage.
  • Integrates CssBaseline with the UnifiedThemeProvider for global style
    normalization.
  • Updates theme import to use backstageTheme from the theme directory.
  • +7/-3     
    backstage-group.yaml
    Add Backstage Group YAML Definition                                           

    examples/acme/backstage-group.yaml

  • Adds a new YAML file defining a Backstage group with metadata and spec
    details.
  • +13/-0   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    johnnyhuy added 27 commits June 9, 2024 00:25
    - Update grid spacing setting in EntityPage component
    - Improve layout by using tighter grid spacing in EntityPage component
    …l-ui.
    
    - Refactor `HomePage.tsx` to use styled components from `@emotion/styled`
    - Utilize material-ui components such as `Box`, `Container`, `Grid`, `Icon`, `Paper`, and `Typography`
    - Enhance the structure and content of the `HomePage` component with structured data and nested components
    - Update route path to "/self-service"
    - Replace `FortRoundedIcon` with `MenuBookRoundedIcon`
    - Add new icons and update sidebar items in Root.tsx
    - Refactor and update file imports, renaming `HomePage` to `Homepage` in `App.tsx`
    - Add new dependencies `@fontsource/inter`, `@mui/joy`, and `@mui/utils` in `package.json`
    - Define new colors, fontFamily variables, and theme settings in `HelloWorld.ts`
    - Refactored theme settings in `HelloWorld.ts` for better organization and readability
    - Updated import statement in `HomePage.tsx` for improved code clarity
    - Update theme in HelloWorld component
    - Improve code structure for theme management
    - Add dependency on `@mui/x-charts` version `7.6.2`
    - Fix typo in component import changing `Homepage` to `HomePage`
    - Refactor various components to use `Box` and `Typography` for improved styling and consistency
    - Added various YAML files containing specifications for new components, APIs, systems, and group configurations
    - Included example entities for Backstage features, domains, groups, users, and resources
    - Updated organization and location information for better organization and metadata clarity
    - Removed unnecessary imports and components
    - Replaced Button component with ToggleButton component
    - Updated styles for better consistency and readability
    - Refactor theme creation logic for better readability
    - Update function signature for `createBackstageTheme`
    - Rename `ThemeOptions` interface to `MuiThemeOptions`
    - Add new imports and update existing components in `HomePage.tsx`
    - Refactor state variable names and update data arrays
    - Create new styled component for search input
    - Update components to reflect boolean values and remove unnecessary imports
    - Added new imports and functions to improve scrolling functionality in the HomePage component
    - Removed unnecessary interface and constant declarations
    - Updated styling and positioning for search and card components
    - Included a new section in the development docs on customizing React components with styled components
    - Added new icon components for Bitbucket, AWS, Slack, Confluence, Azure, Jira, and Discord
    - Implemented SVG rendering for each icon component
    - Defined interface `IconProps` for consistent width and height handling across components
    - Update icons imports and styles in the HomePage component
    - Replace links with Button components for external links in the HomePage component
    - Refactored styles and components in the Home Page
    - Updated content in the Service Health and Cost Overview sections
    - Update exclude value in tsconfig.json
    - Add new key-value pair to compilerOptions object in tsconfig.json
    - Update the home page theme colors to utilize the `rootBeer` color palette
    - Refactor the HomePage component to improve readability and maintainability
    …tories
    
    - Update import path for `backstageTheme` to a folder in `App.tsx`
    - Rename `HelloWorld.ts` to `index.ts` in the `theme` directory
    - Update `HomePage.tsx` for improved styling and functionality
    - Refactor `scrollContainerRef` to use `useRef`
    - Adjust layout and styling of various components
    - Update chart configurations for consistency
    - Improve styling of `CardContent` components in `HomePage.tsx`
    - Enhance display and color of numerical values in `CardContent` components
    - Add `lineElementClasses` import in `LineChart` component
    - Add `id` field in `costSeries` array and `Databases` data object
    - Change text variants in `Typography` component
    - Delete unnecessary styling and components
    - Add TrendingUpIcon and TrendingDownIcon imports
    - Update data objects for Uptime and Downtime in serviceHealthSeries
    - Implement Chips for past 7 days in Service Health and past 6 months in Cost Overview sections
    - Added new icons and updated existing icons in the `HomePage` component
    - Introduced a new section on "Ownership" with corresponding content and icons
    - Included a card on "Backend Services" with updated information and a new icon
    - Refactored the styling in `HomePage.tsx` for better alignment and improved structure
    - Replaced components with a `Box` component for better styling consistency
    - Updated `Button` variant to "text" and removed padding for cleaner appearance
    @johnnyhuy johnnyhuy marked this pull request as ready for review June 26, 2024 12:53
    @johnnyhuy
    Copy link
    Contributor Author

    /describe

    @echohello-codium-ai-pr-agent echohello-codium-ai-pr-agent bot added the enhancement New feature or request label Jun 26, 2024
    @johnnyhuy
    Copy link
    Contributor Author

    /review

    @echohello-codium-ai-pr-agent
    Copy link
    Contributor

    echohello-codium-ai-pr-agent bot commented Jun 26, 2024

    PR Review 🔍

    (Review updated until commit ceb0e2d)

    ⏱️ Estimated effort to review [1-5]

    4, due to the extensive changes across multiple files and components, including the addition of new icon components, adjustments to existing layout and spacing, and the integration of new styling approaches. The changes impact both the visual presentation and the structure of the application, requiring a thorough review of both the code's functionality and its adherence to design and style guidelines.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Layout Inconsistency: The changes in EntityPage.tsx to adjust layout and spacing might cause inconsistencies with other pages or components that have not been adjusted in a similar manner.

    Hardcoded Values: The direct use of specific dimensions and URLs in the icon components (Grafana.tsx, Prometheus.tsx, Aws.tsx) could limit reusability and adaptability to different contexts or future changes.

    🔒 Security concerns

    No

    Code feedback:
    relevant filepackages/app/src/components/icons/Grafana.tsx
    suggestion      

    Consider using a centralized theme or configuration file for managing SVG attributes such as width, height, and viewBox to enhance consistency and maintainability across different icon components. [important]

    relevant lineviewBox="0 0 45 48"

    relevant filepackages/app/src/components/icons/Prometheus.tsx
    suggestion      

    Extract the SVG paths and gradients into separate variables or files to improve readability and facilitate easier updates or reuse across components. [medium]

    relevant linefill="url(#paint0_linear_493_18)"

    relevant filepackages/app/src/components/icons/Aws.tsx
    suggestion      

    Implement an SVG optimization step in your build process or use a tool like SVGO to reduce file size and optimize the SVG code for better performance. [important]

    relevant linefill="#252F3E"

    relevant filepackages/app/src/components/catalog/EntityPage.tsx
    suggestion      

    Use theme spacing from Material-UI's theme provider instead of hardcoded padding and margin values to ensure consistency with the rest of the application's layout. [important]

    relevant line

    @echohello-codium-ai-pr-agent
    Copy link
    Contributor

    PR Description updated to latest commit (ceb0e2d)

    @echohello-codium-ai-pr-agent
    Copy link
    Contributor

    Persistent review updated to latest commit ceb0e2d

    @echohello-codium-ai-pr-agent
    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Rename the icon component to GrafanaIcon for better readability

    Consider using a more descriptive name for the icon component in the HomePage.tsx file.
    The current name icon is too generic and does not convey the purpose or the type of icons
    it represents. A more descriptive name will improve code readability and maintainability.

    packages/app/src/components/home/HomePage.tsx [8]

    -const icon: React.FC<IconProps> = ({ width, height }) => {
    +const GrafanaIcon: React.FC<IconProps> = ({ width, height }) => {
     
    Suggestion importance[1-10]: 7

    Why:

    7
    Abstract theme customization into a separate function or component

    It's recommended to abstract the theme customization logic into a separate function or
    component to keep the HomePage component clean and focused on its primary responsibility.
    This will improve code organization and make it easier to manage theme-related logic.

    packages/app/src/components/home/HomePage.tsx [193-214]

    -<ThemeProvider
    -  theme={(parentTheme: Theme) =>
    -    createTheme({
    -      ...parentTheme,
    -      components: {
    -        MuiFilledInput: {
    -          styleOverrides: {
    -            root: {
    -              '&::before, &::after': {
    -                borderBottom: 'none',
    -              },
    -              '&:hover:not(.Mui-disabled, .Mui-error):before': {
    -                borderBottom: 'none',
    -              },
    -              '&.Mui-focused:after': {
    -                borderBottom: 'none',
    -              },
    -            },
    -          },
    -        },
    -      },
    -    })
    -  }
    ->
    +const customTheme = createCustomTheme(parentTheme);
    +...
    +<ThemeProvider theme={customTheme}>
     
    Suggestion importance[1-10]: 7

    Why:

    7
    Correct the typo in the function name for consistency and readability

    There's a typo in the function name creatBackstageTheme. It should be corrected to
    createBackstageTheme to improve code readability and maintain consistency.

    packages/app/src/theme/index.ts [76]

    -export function creatBackstageTheme(options: ThemeOptions): UnifiedTheme {
    +export function createBackstageTheme(options: ThemeOptions): UnifiedTheme {
     
    Suggestion importance[1-10]: 7

    Why:

    7
    Define SVG paths and gradients as separate components for better code organization

    For better readability and maintainability, consider defining the SVG paths and gradients
    as separate components or variables outside the main component function.

    packages/app/src/components/icons/Confluence.tsx [17-20]

    -<path
    -  d="M2.22879 46.0837C1.57879 47.1437 0.848797 48.3737 0.278797 49.3537C0.0120292 49.8045 -0.0672331 50.342 0.0580516 50.8506C0.183336 51.3592 0.503165 51.7984 0.948795 52.0737L13.9488 60.0737C14.1747 60.2132 14.4261 60.3063 14.6884 60.3476C14.9506 60.3889 15.2185 60.3776 15.4763 60.3143C15.7341 60.251 15.9768 60.137 16.1901 59.9789C16.4034 59.8209 16.5832 59.6219 16.7188 59.3937C17.2288 58.5237 17.8988 57.3937 18.6288 56.1837C23.7788 47.6837 28.9688 48.7237 38.2988 53.1837L51.1888 59.3137C51.4304 59.4287 51.6927 59.4941 51.96 59.5059C52.2274 59.5177 52.4944 59.4757 52.7452 59.3825C52.9961 59.2892 53.2257 59.1466 53.4204 58.963C53.6151 58.7794 53.771 58.5586 53.8788 58.3137L60.0688 44.3137C60.279 43.8331 60.292 43.2892 60.1049 42.7991C59.9179 42.309 59.5458 41.912 59.0688 41.6937C56.3488 40.4137 50.9388 37.8537 46.0688 35.5137C28.5088 26.9737 13.6188 27.5337 2.22879 46.0837Z"
    -  fill="url(#paint0_linear_1_52)"
    -/>
    +const Path1 = () => (
    +  <path
    +    d="M2.22879 46.0837C1.57879 47.1437 0.848797 48.3737 0.278797 49.3537C0.0120292 49.8045 -0.0672331 50.342 0.0580516 50.8506C0.183336 51.3592 0.503165 51.7984 0.948795 52.0737L13.9488 60.0737C14.1747 60.2132 14.4261 60.3063 14.6884 60.3476C14.9506 60.3889 15.2185 60.3776 15.4763 60.3143C15.7341 60.251 15.9768 60.137 16.1901 59.9789C16.4034 59.8209 16.5832 59.6219 16.7188 59.3937C17.2288 58.5237 17.8988 57.3937 18.6288 56.1837C23.7788 47.6837 28.9688 48.7237 38.2988 53.1837L51.1888 59.3137C51.4304 59.4287 51.6927 59.4941 51.96 59.5059C52.2274 59.5177 52.4944 59.4757 52.7452 59.3825C52.9961 59.2892 53.2257 59.1466 53.4204 58.963C53.6151 58.7794 53.771 58.5586 53.8788 58.3137L60.0688 44.3137C60.279 43.8331 60.292 43.2892 60.1049 42.7991C59.9179 42.309 59.5458 41.912 59.0688 41.6937C56.3488 40.4137 50.9388 37.8537 46.0688 35.5137C28.5088 26.9737 13.6188 27.5337 2.22879 46.0837Z"
    +    fill="url(#paint0_linear_1_52)"
    +  />
    +);
     
    Suggestion importance[1-10]: 7

    Why:

    7
    Best practice
    Create a shared utility function for currency formatting to avoid code duplication

    For the valueFormatter function used in the costSeries and potentially other series,
    consider creating a shared utility function. This will avoid code duplication and make it
    easier to apply consistent formatting across different components or sections of the
    application.

    packages/app/src/components/home/HomePage.tsx [137-141]

    -valueFormatter: (value: number | bigint) =>
    -  new Intl.NumberFormat('en-US', {
    -    style: 'currency',
    -    currency: 'USD',
    -  }).format(value),
    +const formatCurrency = (value: number | bigint) => new Intl.NumberFormat('en-US', { style: 'currency', currency: 'USD' }).format(value);
    +...
    +valueFormatter: formatCurrency,
     
    Suggestion importance[1-10]: 7

    Why:

    7
    Apply consistent spacing at the container level for uniform layout spacing

    The removal of spacing from the Grid container and the introduction of padding in
    individual Grid items changes the overall spacing dynamics. This approach might lead to
    inconsistencies in spacing between different sections. Consider applying a uniform spacing
    strategy at the container level to maintain consistency and control over the layout's
    spacing more effectively.

    packages/app/src/components/catalog/EntityPage.tsx [126]

    -<Grid container spacing={0} alignItems="stretch">
    +<Grid container spacing={2} alignItems="stretch">
       {entityWarningContent}
     </Grid>
     
    Suggestion importance[1-10]: 7

    Why:

    7
    Capitalize the functional component name to follow React conventions

    Since the icon component is a functional component, it's recommended to follow the React
    component naming convention by capitalizing the first letter of the component name. Rename
    icon to Icon to align with common React practices.

    packages/app/src/components/icons/Aws.tsx [8]

    -const icon: React.FC<IconProps> = ({ width, height }) => {
    +const Icon: React.FC<IconProps> = ({ width, height }) => {
     
    Suggestion importance[1-10]: 7

    Why:

    7
    Enhancement
    Use dynamic values for scrolling to improve consistency and adaptability

    Instead of hardcoding the scroll amount in the scrollLeft and scrollRight functions,
    consider using a dynamic value based on the width of the container or the cards. This will
    make the scrolling behavior more consistent and adaptable to different screen sizes or
    card sizes.

    packages/app/src/components/home/HomePage.tsx [81-97]

    +const cardWidth = 300; // Example card width, adjust as necessary
     scrollLeft = () => {
       if (scrollContainerRef.current) {
         scrollContainerRef.current.scrollBy({
    -      left: -300, // Adjust the scroll amount as needed
    +      left: -cardWidth,
           behavior: 'smooth',
         });
       }
     };
     scrollRight = () => {
       if (scrollContainerRef.current) {
         scrollContainerRef.current.scrollBy({
    -      left: 300, // Adjust the scroll amount as needed
    +      left: cardWidth,
           behavior: 'smooth',
         });
       }
     };
     
    Suggestion importance[1-10]: 7

    Why:

    7
    Use explicit values for the xs prop to improve layout predictability

    For better accessibility and to ensure that the layout scales well across different screen
    sizes, consider explicitly setting the xs prop to a specific value instead of using it as
    a boolean. This change aims to improve the predictability of the grid layout's behavior on
    extra-small screens.

    packages/app/src/components/catalog/EntityPage.tsx [101]

    -<Grid item xs padding={1}>
    +<Grid item xs={12} padding={1}>
       <EntityOrphanWarning />
     </Grid>
     
    Suggestion importance[1-10]: 7

    Why:

    7
    Use React.SVGProps for SVG component props for greater flexibility

    Consider using React.SVGProps for the props of your SVG component instead of defining a
    custom IconProps interface. This change will make your component more flexible by allowing
    it to accept all SVG properties, not just width and height.

    packages/app/src/components/icons/Aws.tsx [3-6]

    -interface IconProps {
    -  width: number;
    -  height: number;
    -}
    +const icon: React.FC<React.SVGProps<SVGSVGElement>> = (props) => {
     
    Suggestion importance[1-10]: 7

    Why:

    7
    Add dynamic color generation based on theme mode for enhanced customization

    To enhance the theme customization and scalability, consider adding a mechanism to
    dynamically generate the colors object based on a theme mode (e.g., light or dark). This
    approach allows for easier adjustments and additions to the theme colors in the future.

    packages/app/src/theme/index.ts [24-30]

    -const colors = {
    +const getColors = (mode: 'light' | 'dark') => ({
       darkTangerine: '#FFA217',
    -  gold: '#EBC387',
    +  gold: mode === 'dark' ? '#D9A567' : '#EBC387',
       pastelOrange: '#F9B249',
    -  floralWhite: '#FEF8EF',
    +  floralWhite: mode === 'dark' ? '#E8E0D9' : '#FEF8EF',
       rootBeer: '#1D1301',
    -};
    +});
    +const colors = getColors('light'); // or 'dark', based on user preference or system settings
     
    Suggestion importance[1-10]: 7

    Why:

    7
    Possible issue
    Ensure consistent and responsive padding across Grid items

    Consider using consistent padding across all Grid items for a uniform appearance. The
    current implementation uses padding={1} for all Grid items, which is a change from the
    previous xs={12} setting. If the intention was to reduce spacing, it's important to ensure
    that this does not negatively impact the layout on different screen sizes. Testing across
    various devices or screen widths can help ensure the layout remains responsive and
    visually appealing.

    packages/app/src/components/catalog/EntityPage.tsx [101]

    -<Grid item xs padding={1}>
    +<Grid item xs padding={2}>
       <EntityOrphanWarning />
     </Grid>
     
    Suggestion importance[1-10]: 7

    Why:

    7
    Reevaluate the removal of vertical margins between components for clarity

    Removing the marginBottom prop from Grid items might lead to layout issues where vertical
    spacing between components becomes too compressed, especially in a dashboard layout where
    separation between elements is crucial for readability. Consider testing the visual impact
    of this change thoroughly or reintroducing some form of vertical margin or spacing to
    maintain clear separation between components.

    packages/app/src/components/catalog/EntityPage.tsx [128]

    -<Grid item md={6} xs padding={1} marginBottom={-1}>
    +<Grid item md={6} xs padding={1} style={{ marginBottom: '16px' }}>
       <EntityAboutCard variant="gridItem" />
     </Grid>
     
    Suggestion importance[1-10]: 7

    Why:

    7
    Performance
    Use React.memo to prevent unnecessary re-renders of the icon component

    Consider using React.memo for the icon component to avoid unnecessary re-renders. This is
    particularly useful if the icon components are frequently re-rendered with the same props.

    packages/app/src/components/icons/Confluence.tsx [8]

    -const icon: React.FC<IconProps> = ({ width, height }) => {
    +const icon: React.FC<IconProps> = React.memo(({ width, height }) => {
     
    Suggestion importance[1-10]: 7

    Why:

    7

    - Update padding in Grid items and containers in EntityPage.tsx
    - Fix typo in createBackstageTheme function name in theme/index.ts
    - Add style overrides for MuiGrid in backstageTheme constant
    @johnnyhuy johnnyhuy enabled auto-merge June 28, 2024 14:30
    @johnnyhuy johnnyhuy disabled auto-merge June 29, 2024 00:45
    @johnnyhuy johnnyhuy enabled auto-merge June 29, 2024 00:45
    @johnnyhuy johnnyhuy disabled auto-merge June 29, 2024 00:45
    @johnnyhuy johnnyhuy merged commit 5e63e80 into main Jun 29, 2024
    @johnnyhuy johnnyhuy deleted the feature/homepage-v2 branch June 29, 2024 00:45
    @johnnyhuy johnnyhuy restored the feature/homepage-v2 branch June 30, 2024 14:40
    johnnyhuy added a commit that referenced this pull request Jun 30, 2024
    @johnnyhuy johnnyhuy deleted the feature/homepage-v2 branch June 30, 2024 14:52
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant