[GitHub] [incubator-superset] rusackas commented on pull request #12122: refactor: Bootstrap to AntD - Alert (#12101)

[GitBox <gi...@apache.org>]

rusackas commented on pull request #12122:
URL: https://github.com/apache/incubator-superset/pull/12122#issuecomment-754296259


   > @rusackas Those were great observations. For me these specific properties (margins) are related to positioning of the component in a screen so they are not generic but layout based. With that in mind, Alert already have properties for this scenario: `style` and `className`. What `styled` is actually doing is passing a `className` for the Alert component but in a non direct manner so I thought of replacing that with the `css` property.
   > 
   > ```
   > <Alert
   >      css={theme => ({
   >        marginTop: theme.gridUnit * 4,
   >        marginBottom: theme.gridUnit * 4,
   >      })}
   >      ...
   > />
   > ```
   > 
   > This makes more clear the we are positioning the element and have the benefit of autocompletion and type checking.
   > 
   > You were totally wright about the mismatching grid units. I changed all to 4.
   > 
   > Let me know if you agree with that 😉
   
   If these are one-off, situational tweaks, then I prefer the way you have it here, using the `css` prop. Somehow that looks more like a contextual tweak, whereas `styled` makes it look (to me, at least) more like we're creating a new variant of the component. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org