Feature/modal#114
Conversation
RupeshAagde-fynd
commented
Feb 23, 2023
RupeshAagde-fynd
commented
|
Props added |
|
@RupeshAagde-fynd , please add tests. |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
@RupeshAagde-fynd please add test cases
| padding: 0 24px; | ||
| height: 40px; | ||
| // height: 40px; | ||
| line-height: 24px; |
There was a problem hiding this comment.
@RupeshAagde-fynd please change all hardcoded pixels to rem irrespective of previous implementations
| </n-dialog> | ||
| </div> | ||
| `, | ||
| data: function () { |
There was a problem hiding this comment.
why do we have empty state? if not required, please remove it
| @@ -1,10 +1,13 @@ | |||
| <template> | |||
| <div v-show="isModalVisible" :id="id"> | |||
There was a problem hiding this comment.
Why was this v-show removed? Wouldn't it break existing implementations of the dialog? Please review and fix
| <footer class="nitrozen-dialog-footer"> | ||
| <slot name="footer"> | ||
| <nitrozen-button | ||
| v-if="positiveButtonLabel" |
There was a problem hiding this comment.
Please maintain backward compatibility for existing props. This will break any implementations using this prop
| neutralButtonLabel: "Ok", | ||
| positiveButtonLabel: false, | ||
| neutralButtonLabel: "ok", | ||
| positiveButtonLabel: "cancel", |
There was a problem hiding this comment.
Any reason we changed the default values? If yes, please specify
| margin: 24px 0; | ||
| padding: 0 24px; | ||
| height: 40px; | ||
| // height: 40px; |
| @@ -1,13 +1,13 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <svg width="10px" height="8px" viewBox="0 0 10 8" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> | |||
| <svg width="16px" height="16px" viewBox="0 0 10 8" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> | |||
There was a problem hiding this comment.
Why was this SVG edited? We have to use the new icons specified in the library instead of using the old ones. If this is needed, please specify reason
1 participant
