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

[CCI] Fix bottom notification bar width #3804

Conversation

SergeyMyssak
Copy link
Contributor

Description

Fix the width of the bottom notification bar.

It is not possible to override the width without !important because the OuiBottomBar component uses inline styles. We have the option of not using !important in our styles, but then we would have to remove the default inline styles in the OuiBottomBar component so we have more flexibility with the styles.

Screen.Recording.2023-04-07.at.16.54.12.mov

Issues Resolved

#3336

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@SergeyMyssak SergeyMyssak requested a review from a team as a code owner April 7, 2023 10:56
@SergeyMyssak SergeyMyssak force-pushed the 3336-Fix-bottom-notification-bar-width branch from c1b3700 to 006cc46 Compare April 7, 2023 10:57
@ashwin-pc
Copy link
Member

@SergeyMyssak Thanks for looking at this. My main concern with this PR is that it is a fixed width that we are applying to mask this behaviour. What we want is this:

  1. When the flyout is not docked (The lock icon at the bottom is not selected) nothing happens
  2. When the flyout is docked, the bottombar takes into account the flyout width.

also, can we use the relative position of the bottom bar with respect to the flyout to auto adjust the width, instead of relying on a fixed width solution?

@joshuarrrr
Copy link
Member

but then we would have to remove the default inline styles in the OuiBottomBar component so we have more flexibility with the styles.

@SergeyMyssak Do you mind opening an issue in OUI to remove those?
https://github.com/opensearch-project/oui/issues

@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2023

Codecov Report

Merging #3804 (25a26fd) into main (755f16b) will decrease coverage by 0.01%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3804      +/-   ##
==========================================
- Coverage   66.44%   66.43%   -0.01%     
==========================================
  Files        3229     3229              
  Lines       62068    62068              
  Branches     9599     9599              
==========================================
- Hits        41238    41234       -4     
- Misses      18527    18530       +3     
- Partials     2303     2304       +1     
Flag Coverage Δ
Linux 66.37% <ø> (-0.01%) ⬇️
Windows 66.38% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@joshuarrrr
Copy link
Member

@SergeyMyssak I'm marking this as a draft for now - we can revisit after getting the OUI changes merged first.

@joshuarrrr joshuarrrr marked this pull request as draft April 14, 2023 22:36
Co-authored-by: Andrey Myssak <andreymyssak@gmail.com>
Signed-off-by: Sergey Myssak <sergey.myssak@gmail.com>
@SergeyMyssak SergeyMyssak force-pushed the 3336-Fix-bottom-notification-bar-width branch from fd09fb1 to 25a26fd Compare April 26, 2023 08:32
@SergeyMyssak
Copy link
Contributor Author

Closing this in favour of #3978

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time-contributor OSCI Open Source Contributor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants