Add dismissible prop to OuiCallOut #985
Conversation
Signed-off-by: abbyhu2000 <[email protected]>
abbyhu2000
requested review from
seanneumann,
AMoo-Miki,
rednaksi91,
KrooshalUX,
BSFishy,
bandinib-amzn,
ashwin-pc,
joshuarrrr,
andreymyssak and
SergeyMyssak
as code owners
Signed-off-by: abbyhu2000 <[email protected]>
src/components/call_out/call_out.tsx
Outdated
| ...rest | ||
| }, | ||
| ref: Ref<HTMLDivElement> | ||
| ) => { | ||
| const [isCalloutVisible, setIsCalloutVisible] = useState(true); |
There was a problem hiding this comment.
Can we persist this value in session storage?
There was a problem hiding this comment.
I don't think there is any precedence of using any sort of storage in OUI. In fact, I feel like a more common pattern would be to have a prop like showDismissIcon and onDismiss and let the builder handle the functionality. @KrooshalUX input might be helpful here
|
@abbyhu2000 I assume you'll need this for OSD 2.10 release? If so, can you add it to #958? |
Signed-off-by: abbyhu2000 <[email protected]>
|
|
||
| .ouiCallOut__closeIcon { | ||
| position: absolute; | ||
| right: 6px; |
There was a problem hiding this comment.
I feel like this should use $ouiSize in some way. Maybe calc($ouiSizeM / 2)?
There was a problem hiding this comment.
Changed to $ouiSizeS / 2
src/components/call_out/call_out.tsx
Outdated
| ...rest | ||
| }, | ||
| ref: Ref<HTMLDivElement> | ||
| ) => { | ||
| const [isCalloutVisible, setIsCalloutVisible] = useState(true); |
There was a problem hiding this comment.
I don't think there is any precedence of using any sort of storage in OUI. In fact, I feel like a more common pattern would be to have a prop like showDismissIcon and onDismiss and let the builder handle the functionality. @KrooshalUX input might be helpful here
|
@BSFishy Yea I called it out in the original issue too about making i am okay with either implementation. |
OUI is already far from a purely stylistic component library with many of its components having a lot of state information stored. While i do agree that we can control the state from outside the component, the reason I asked Abby to implement it this way was to make using this feature as easy as possible for the average developer. If in future we want to relegate control to outside the application, we can always expand the capability to do so. e.g. This is similar to React's controlled and uncontrolled components philosophy. |
Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: abbyhu2000 <[email protected]>
* add dismissible prop to OuiCallOut Signed-off-by: abbyhu2000 <[email protected]> * changelog Signed-off-by: abbyhu2000 <[email protected]> * fix some nits Signed-off-by: abbyhu2000 <[email protected]> * change size to oui size Signed-off-by: abbyhu2000 <[email protected]> * no state management in oui Signed-off-by: abbyhu2000 <[email protected]> --------- Signed-off-by: abbyhu2000 <[email protected]> (cherry picked from commit 53ad3e7) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
* add dismissible prop to OuiCallOut Signed-off-by: abbyhu2000 <[email protected]> * changelog Signed-off-by: abbyhu2000 <[email protected]> * fix some nits Signed-off-by: abbyhu2000 <[email protected]> * change size to oui size Signed-off-by: abbyhu2000 <[email protected]> * no state management in oui Signed-off-by: abbyhu2000 <[email protected]> --------- Signed-off-by: abbyhu2000 <[email protected]> (cherry picked from commit 53ad3e7) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
Add optional prop
dismissibletoOuiCallOut.Screen.Recording.2023-08-23.at.3.06.26.PM.mov
Issues Resolved
resolves #881
Check List
yarn lintyarn test-unitBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.