Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

style: Optimize style and layout #7671

Merged
merged 2 commits into from
Jan 8, 2025
Merged

style: Optimize style and layout #7671

merged 2 commits into from
Jan 8, 2025

Conversation

lan-yonghui
Copy link
Member

No description provided.

Copy link

f2c-ci-robot bot commented Jan 8, 2025

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

</el-col>
</el-row>
</div>
</div>
<div class="h-app-divider" />
</div>
</el-scrollbar>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated code has several improvements that enhance readability, maintainability, and performance:

  1. Flexbox Layout: The use of display: flex replaces nested el-row and el-col to improve layout structure. This makes the code cleaner and easier to understand.

  2. Grid Removal: The <el-grid> element is removed, simplifying the divs structure for better styling manipulation.

  3. Code Refactoring:

    • A function call $t('app.install') is moved into a single line inside the button template literal.
    • Spacing adjustments are made to ensure consistent formatting around braces {} within Vue template interpolations.
  4. Optimization:

    • Removed unnecessary comments as they clutter the code without adding value to understanding its functionality.

Overall, this change enhances the overall appearance and functionality while maintaining a clean development pattern.

@@ -15,7 +15,7 @@
</el-badge>
</el-radio-button>
</el-radio-group>
<div class="flex flex-row gap-2 md:flex-col lg:flex-row">
<div class="flex flex-col gap-2 sm:flex-row">
<slot name="route-button"></slot>
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After checking the code differences, I identified several irregularities and potential issues:

  1. HTML Tags: The flex classes are incorrectly written with hyphens (sm) instead of camel case (sm:). This should be corrected to use colon notation like sm:flex.

  2. Slot Content Placement: In the slot content, using a conditional statement directly inside it can lead to unexpected behavior because slots don't naturally support direct JavaScript logic within their contents. Consider extracting this logic outside the template if possible.

  3. Card Padding: Removing padding from .el-card might not be desired, and increasing padding could improve readability or aesthetics.

  4. Flex Direction: Aligning buttons vertically by default can change the layout structure unnecessarily. If needed, consider changing the alignment only when necessary (e.g., on screens smaller than medium).

Here's an improved version of the code addressing these points:

<template>
    <el-card class="router_card">
        <div class="w-full flex flex-grow flex-col items-center justify-end gap-y-4 sm:flex-row">
            <!-- Radio group for displaying routes -->
            <el-radio-group v-model="activeName" @change="handleChange">
                <el-radio-button 
                    class="router_card_button px-4 py-2"
                    :disabled="!showRouteButton"
                    >{route.name}
                        <span>{route.icon}</span></el-radio-button>
                </el-radio-button>        
            </el-radio-group>

            <!-- Slot for additional route buttons or actions -->
            <div class="flex flex-gap-2 lg:hidden">                
                <slot name="extra-actions"></slot>
            </div>
            
            <!-- Additional div for slot content at mobile sizes -->
            <div class="hidden md:block">
                <slot name="extra-actions"></slot>
            </div>
       </div>
   </el-card>
</template>

<script lang='ts'>
import { ElCard, ElRadioGroup, ElRadioButton } from 'element-plus';
export default {
    components: {
      ElCard,
      ElRadioGroup,
      ElRadioButton
    },
    data() {
        return {
          activeName: '',
          showRouteButton: true // Example variable controlling visibility of the button
        };
    },
    methods: {
      handleChange(name) {
         console.log('Active Name:', name);
      }
    }
};
</script>

<style scoped lang='scss'>

.router_card {
    height:auto;
    position:relative;

}

// Responsive styling examples
@media screen and (max-width:900px){
    .el-card{
     display:none; /* Adjust based on your requirements */
    }

    .btn_container {

     .btn_item{

           background-color:#fff5ec;
            margin-right:2% !important;

         &.selected , &:focus-within{
              background-image:url('@/assets/images/checked.png');
              border-bottom-left-radius:100%!important ;
              border-bottom-right-radius:100%!important ;
               border-top:1px solid black;
           }
     }
    }
    
   
}

/* Other styles as per project design */

This revised code maintains correct HTML tag formatting, improves slot usage and logic extraction, adjusts card padding according to need, and offers basic responsiveness adjustments through media queries. Make sure to customize any other styles as appropriate for your application's design needs.

@@ -243,6 +243,6 @@ defineExpose({
border: 1px solid var(--el-color-warning);
background-color: transparent;
padding: 8px 8px;
width: 78px;
max-width: 100px;
}
</style>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the provided code snippet, there is one minor change that can be made to enhance readability:

Change from:

width: 78px;

To:

max-width: 100px;

Explanation:

  • Original Code: The width property is set directly to 78px.
  • Proposed Change: Changing it to max-width: 100px; allows the element to expand up to its maximum allowable width while maintaining responsiveness and not exceeding the specified size.

This suggests a preference for flexibility without setting an exact pixel limit.

</el-col>
</el-row>
</div>
</div>
<div class="h-app-divider" />
</div>
</el-scrollbar>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several improvements that can be made to this code:

  1. Consolidate Row Spans: The original code uses both el-col with column spans and an inline div. It would be clearer to switch to using solely el-col with appropriate span settings.

  2. Improve Flexbox Structure: Use CSS Grid instead of combined Flexbox layouts where possible because it handles grid alignment more gracefully.

  3. Remove Extra Elements: The <div> around each row element is unnecessary, making the structure cleaner.

Here's how the improved version could look:

<div class="flex justify-start">
    <el-scrollbar height="525px" class="moz-height">
        <div class="h-app-card w-full" v-for="(app, index) in apps" :key="index">
            <div class="grid grid-cols-2 items-center space-x-2 grow">
                <div>
                    <el-avatar shape="square" :size="55" :src="'data:image/png;base64,' + app.icon" />
                </div>
                <div class="grow">
                    <div class="flex flex-col h-app-content">
                        <span class="h-app-title">{{ app.name }}</span>
                        <div class="h-app-desc">
                            <span>{{ language == 'zh' || language == 'tw' ? app.shortDescZh : app.shortDescEn }}</span>
                        </div>
                    </div>
                </div>
                <div>
                    <el-button class="h-app-button" type="primary" @click="installApp(app)">Install</el-button>
                </div>
            </div>
            <div class="h-app-divider" />
        </div>
    </el-scrollbar>
</div>

Key Improvements:

  • Use Grid Layout: This makes the layout more organized and easier to understand.
  • Simplify HTML Structure:Removed extra <div> elements for better readability and consistency.
  • Flex Properties: Used flex properties within .grow classes for dynamic sizing relative to surrounding content.

This approach maintains similar functionality while improving overall aesthetics and maintainability of the codebase.

@@ -15,7 +15,7 @@
</el-badge>
</el-radio-button>
</el-radio-group>
<div class="flex flex-row gap-2 md:flex-col lg:flex-row">
<div class="flex flex-col gap-2 sm:flex-row">
<slot name="route-button"></slot>
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. In the <el-card> element, consider adding padding (p-1) instead of margin for better spacing consistency.
  2. For smaller screens (sm), reduce the horizontal margins to sm:p-0. Adjusting paddings based on screen sizes helps maintain consistent visual appearance across various devices.
  3. The .md:justify-between and .lg:flex-row classes can be consolidated into a single .mx-auto class to center content more efficiently while maintaining responsiveness.

Here's the optimized solution:

<template>
    <el-card class="router_card p-1 sm:p-0">
        <div class="flex w-full flex-col justify-start sm:items-center items-start mx-auto sm:justify-between sm:flex-row">
            <el-radio-group v-model="activeName" @change="handleChange">
                ...
            </el-radio-group>
            <div class="flex flex-col gap-2 sm:flex-row">
                ...
            </div>
        </div>
    </el-card>
</template>

<script>
export default {
    data() {
        return {
            activeName: ""
        };
    },
    methods: {
        handleChange(name) {
            console.log(`Selected tab: ${name}`);
        }
    }
};
</script>

These changes improve readability and adaptibility by ensuring uniform styling and enhanced user experience across different device sizes.

@@ -243,6 +243,6 @@ defineExpose({
border: 1px solid var(--el-color-warning);
background-color: transparent;
padding: 8px 8px;
width: 78px;
max-width: 100px;
}
</style>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes suggest that the element's width has been changed from 78px to max-width: 100px. This should not be modified without careful consideration as it might affect the layout of other elements on the page, potentially causing overflow or responsiveness problems.

Copy link

sonarqubecloud bot commented Jan 8, 2025

Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@wanghe-fit2cloud
Copy link
Member

/approve

Copy link

f2c-ci-robot bot commented Jan 8, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wanghe-fit2cloud

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@f2c-ci-robot f2c-ci-robot bot added the approved label Jan 8, 2025
@f2c-ci-robot f2c-ci-robot bot merged commit a82a4eb into dev Jan 8, 2025
6 checks passed
@f2c-ci-robot f2c-ci-robot bot deleted the pr@dev@feat_style branch January 8, 2025 10:19
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.

3 participants