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

fix(comp:modal): style update and css variable support #1479

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

danranVm
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added/updated or not needed
  • Docs and demo have been added/updated or not needed

What is the current behavior?

What is the new behavior?

Other information

@@ -1 +1 @@
@mask-background-color: fade(@color-black, 40%);
@mask-background-color: fade(#000, 45%);
Copy link

Choose a reason for hiding this comment

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

with the review

  1. Syntax: The syntax of the code appears to be correct.

  2. Logic: It looks like the patch is attempting to change the mask-background-color from a color-black fade to a #000 fade. This appears to make sense and should not have any bug risks associated with it.

  3. Improvements: The code patch could be improved by using variables instead of hardcoded values, if possible. Additionally, it could be helpful to include comments explaining what the code does in order to make it easier to read and maintain.

@idux-bot
Copy link

idux-bot bot commented Feb 28, 2023

This preview will be available after the AzureCI is passed.

@@ -3,7 +3,7 @@
| `@desc-item-height-sm` | `var(--ix-height-sm)` | - | - |
| `@desc-item-height-md` | `var(--ix-height-md)` | - | - |
| `@desc-item-height-lg` | `var(--ix-height-lg)` | - | - |
| `@desc-item-font-size-sm` | `var(--ix-font-size-sm)` | `var(--ix-font-size-sm)` | - |
| `@desc-item-font-size-sm` | `var(--ix-font-size-sm)` | - | - |
| `@desc-item-font-size-md` | `var(--ix-font-size-md)` | `var(--ix-font-size-sm)` | - |
| `@desc-item-font-size-lg` | `var(--ix-font-size-lg)` | `var(--ix-font-size-sm)` | - |
| `@desc-item-label-color` | `var(--ix-text-color-info)` | - | - |
Copy link

Choose a reason for hiding this comment

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

the code review:

  1. The code patch seems to be related to a set of css variables.
  2. It looks like the patch is removing the default values for some css variables, which might cause unintended behavior.
  3. We should make sure that all the css variables have meaningful default values and their values don't override each other.
  4. We should also consider adding validation for the values of the css variables to ensure they are valid.

mode: 'primary',
onClick: () => modalRef.ok(),
},
asyncButton,
],
})
}
Copy link

Choose a reason for hiding this comment

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

:

  1. import reactive
    The code adds the reactive function from Vue, which is used to create an asyncButton object. It can help us better organize the code and make it more readable.
  2. ModalButtonProps
    The code adds the interface ModalButtonProps to specify the type of the asyncButton object. This will help us catch bugs early and make the code easier to read.
  3. openModal2
    In openModal2, the code adds a new button with the loading property and onClick function. The loading property can be used to indicate whether the data is loading or not and the onClick function can be used to trigger the loading animation and complete the asynchronous request.
  4. confirm
    The code adds a new button to the confirm function to add the asyncButton object. This will allow us to customize the button properties and handle the asynchronous requests more efficiently.

@@ -3,6 +3,6 @@ category: components
type: 反馈
title: Modal
subtitle: 对话框
order: 0
theme: true
---

Copy link

Choose a reason for hiding this comment

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

:

The code patch looks like it is intended to add a "theme" property to the modal component.

Some potential issues that could arise from this code patch:

  • The code does not appear to add any validation for the "theme" property, so any value entered by the user could cause unexpected results.
  • There is no indication of what type of data is expected for "theme". Is it a boolean, string, or something else?
  • It is unclear if the code adds any other properties or methods related to the "theme" property.

Suggestions for improvement:

  • Add validation for the "theme" property.
  • Clearly indicate the expected data type for "theme".
  • Add any other necessary properties or methods related to the "theme" property.
  • Make sure to update any other existing code that references the "theme" property.

| `@modal-body-info-color` | `var(--ix-color-primary)` | `var(--ix-color-primary-l10)` | - |
| `@modal-body-success-color` | `var(--ix-color-turquoise)` | - | - |
| `@modal-body-warning-color` | `var(--ix-color-brown)` | `var(--ix-color-brown-l10)` | - |
| `@modal-body-error-color` | `var(--ix-color-red)` | `var(--ix-color-red-l10)` | - |
Copy link

Choose a reason for hiding this comment

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

the code review:

  1. The code patch looks reasonable and it appears that all the variables have been updated to use the new "var()" syntax.
  2. It would be good to check if all the updated variables are being used in the correct places.
  3. Since the '@modal-border-radius' variable has been added, it should be verified that it is being used in the expected places.
  4. Also, it would be good to check if all the variables are correctly scoped and are not being overridden by other variables.
  5. Finally, it would be helpful to add comments to the code to explain what each variable is used for.

@@ -180,6 +180,7 @@ export default defineComponent({
closable={closable.value}
closeIcon={closeIcon.value}
header={props.header}
size={props.type === 'default' ? 'md' : 'sm'}
onClose={close}
/>
<ModalBody></ModalBody>
Copy link

Choose a reason for hiding this comment

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

review

  1. The patch looks good, it adds size prop to the Modal component with a condition check.
  2. This addition is good because it allows us to have different sizes of Modal based on the type.
  3. However, it is important to add a default value for size if none is provided. This will ensure that the code is robust.
  4. There might also be a need to add some validations to check if the type is valid or not.
  5. Additionally, it might be beneficial to add a comment before this lines explaining the purpose of this addition. This will help other developers understand the code better.

max-width: @modal-max-width-screen-sm;
margin: @modal-margin-screen-sm;
max-width: calc(100vw - 16px);
margin: 8px auto;
}

.@{modal-prefix}-centered {
Copy link

Choose a reason for hiding this comment

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

with code review:

  1. The patch changes the max-width of the modal from @modal-max-width to calc(100vw - 32px). This change should make the modal responsive on different screen sizes, so this is a good improvement.

  2. The patch changes the padding of the modal header from var(--ix-spacing-lg) to 16px. This should make the modal look more consistent with other elements on the page.

  3. The patch also changes the font size of the modal body title from @modal-body-title-font-size to .reset-font-size(@modal-body-title-font-size), which should make the title size more consistent with other titles on the page.

  4. The patch changes the margin bottom of the modal body title from @modal-body-title-margin-bottom to 4px, which should make the title size more consistent with other titles on the page.

  5. The patch also changes the padding of the modal body content from @modal-body-content-padding to 8px 16px, which should make the content look more consistent with other elements on the page.

  6. The patch also changes the margin of the modal body icon from @modal-body-icon-margin to 0 16px, which should make the icon look more consistent with other elements on the page.

  7. The patch changes the padding of the modal footer from var(--ix-spacing-md) var(--ix-spacing-lg) to 12px 16px, which should make the footer look more consistent with other elements on the page.

  8. The patch also changes the maximum width of the modal from @modal-max-width-screen-sm to calc(100vw - 16px) and the margin from @modal-margin-screen-sm to 8px auto, which should make the modal responsive on different screen sizes.

Overall, this patch looks good and should improve the look and responsiveness of the modal. However, it would be beneficial to test the changes to make sure they are working as expected.

@modal-body-info-color: var(--ix-color-primary);
@modal-body-success-color: var(--ix-color-turquoise);
@modal-body-warning-color: var(--ix-color-brown);
@modal-body-error-color: var(--ix-color-red);
Copy link

Choose a reason for hiding this comment

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

with the code review

  1. The code patch decreased the number of variables and replaced them with custom variables. This is a good approach as it helps to maintain the same styling and keeps the code more organized.
  2. There are also some additional variables added which will help in making the code even more organized.
  3. The modal-body-title-color has been changed to ix-text-color-title which will help in defining the modal title color better.
  4. The modal-body-confirm-color, modal-body-info-color, modal-body-success-color, modal-body-warning-color and modal-body-error-color have been replaced with custom variables to make the code more organized.
  5. The modal-max-width-screen-sm and modal-margin-screen-sm variables have been removed which is fine since they were not needed.
    Overall, the code looks good and there are no major issues.

@modal-body-confirm-color: var(--ix-color-brown-l10);
@modal-body-info-color: var(--ix-color-primary-l10);
@modal-body-warning-color: var(--ix-color-brown-l10);
@modal-body-error-color: var(--ix-color-red-l10);
Copy link

Choose a reason for hiding this comment

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

with the code review:

  1. The code patch updates the font-weight, color, padding for modal body title and modal body content.
  2. It also updates the min-height, padding of modal footer and colors of modal body confirm, info, warning and error.
  3. The code patch ensures that all variables are updated to use the defined css custom properties for color and font weight.
  4. The code patch also removes redundant and unused variables.

It looks good overall. There are no apparent bugs or risks. The only improvement suggestion would be that the code should be written in a more organized manner, with more comments to explain what each line is doing.

--ix-color-brown: @color-brown;
--ix-color-brown-l10: @color-brown-l10;
--ix-color-brown-d10: @color-brown-d10;

// Text
--ix-text-color: @color-grey-d40; // 默认文字颜色
Copy link

Choose a reason for hiding this comment

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

with a brief overview of the code:

This code patch is adding two new color variables for turquoise and brown, as well as updating the existing ones. The new color variables for turquoise include --ix-color-turquoise-l10 and --ix-color-turquoise-d10, while the new color variables for brown include --ix-color-brown-l10 and --ix-color-brown-d10. The existing variables are also updated accordingly.

Now, let's look at possible risks and improvement suggestions:

Risks:

  1. There may be an issue with the syntax of the added variables, which could cause an error in the code.
  2. The new colors may not match the existing colors, causing inconsistencies in the design.

Improvement Suggestions:

  1. Use a linter to check the syntax of the code to ensure there are no errors.
  2. Double check that the new colors match the existing colors to maintain consistency.

@@ -10,6 +10,8 @@
| `@pro-tree-header-wrapper-icon-font-size` | `@font-size-lg` | - | - |
| `@pro-tree-header-wrapper-icon-hover-color` | `@color-primary` | - | - |
| `@pro-tree-header-wrapper-height` | `38px` | - | - |
| `@pro-tree-content-horizontal-spacing` | `12px` | - | - |
| `@pro-tree-content-vertical-spacing` | `0` | - | - |
| `@pro-tree-divider-horizontal-spacing` | `@pro-tree-header-search-wrapper-horizontal-spacing` | - | - |
| `@pro-tree-divider-vertical-spacing` | `8px` | - | - |
| `@pro-tree-node-padding` | `0 0 0 8px` | - | - |
Copy link

Choose a reason for hiding this comment

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

the review.
The patch looks good, the code seems to be properly formatted and there are no syntax errors. The new variables that have been added seem to be relevant and appropriate. It is not clear what the purpose of these variables is, but they seem to be related to tree content spacing. As such, there is no obvious risk or bug present in the code.

However, one suggestion for improvement would be to add more detailed comments and documentation to the code. This will help future developers who need to make changes to the code understand what the purpose of the variables is and how it should be used. Additionally, if there are any assumptions or special circumstances that need to be taken into account when using the variables, these should also be documented.

@danranVm danranVm merged commit f45776d into IDuxFE:main Feb 28, 2023
@danranVm danranVm deleted the fix-modal-style branch February 28, 2023 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant