[EuiPopover] Fix filter drop-shadow bug#6604
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6604/ |
daveyholler
left a comment
There was a problem hiding this comment.
Visually, looks good in Safari.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6604/ |
I really appreciated this side by side comparison, thanks for going above and beyond with it Elizabet!
Can you explain really quickly why the new example was added as part of this fix? It's not totally clear to me (as the issue was already reproducible in our existing docs). In general I'd prefer we not add (potentially) unrelated changes in the same PR as fixes. If, for whatever reason, this fix needs to be rolled back, this additional doc example may not need to be rolled back with it - so if it's separate, we should include it in a separate PR. |
| ]; | ||
|
|
||
| if (property === 'filter') { | ||
| return `filter: ${array.reduce((v, i) => `${v} drop-shadow(${i})`, '')};`; | ||
| // To prevent Safari from rendering multiple weird shadows we can only use one drop-shadow |
There was a problem hiding this comment.
I can't even seem to find a documented bug report for this anywhere on the internet, so there's no issue we can track to restore previous functionality if Safari ever gets its shit together 🥲 Ah well, it do be that way sometimes
There was a problem hiding this comment.
@cee-chen it's even difficult to find examples on the internet with multiple filter drop-shadow(). My guess is that these shadows were copied and pasted from Figma:
IMO if we can visually achieve the same without multiple drop-shadow the performance will be better. So even if this is fixed in Safari, I don't think we should restore it.
There was a problem hiding this comment.
Interesting! I really like the point you made about performance. In that case, should we update the comment to reflect that, and maybe also move the const array = (or remove it entirely) since it only lives in the box-shadow CSS?
There was a problem hiding this comment.
To be honest I also personally kinda wonder if the multiple box-shadows even add all that much or if we should consider paring them down to just 1 as well...
cee-chen
left a comment
There was a problem hiding this comment.
I'm good with this as-is once rebased against latest main - my previous comment was primarily one of best practices, but it's not a blocking comment in the least.
Awesome work on this Elizabet - this is a super elegant solution with minimal to no tradeoffs 👏
@cee-chen all the reproducible examples in our docs are popovers with a selectable inside. But the UnifiedSearch example from Kibana is a popover with a Then I noticed that the custom example in the context menu page was very poor. All the other examples are real-world examples except this one:
So I just replaced it with the example that I was using for testing. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6604/ |
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6604/ |
## Summary EUI `75.1.1` ➡️ `75.1.2` ## [`75.1.2`](https://github.com/elastic/eui/tree/v75.1.2) **Bug fixes** - Fixed bug in `EuiPopover` where multiple filter `drop-shadow()` were causing inner shadows in Safari ([#6604](elastic/eui#6604))
## Summary EUI `75.1.1` ➡️ `75.1.2` ## [`75.1.2`](https://github.com/elastic/eui/tree/v75.1.2) **Bug fixes** - Fixed bug in `EuiPopover` where multiple filter `drop-shadow()` were causing inner shadows in Safari ([elastic#6604](elastic/eui#6604))
Summary
This PR fixes a bug in Safari where multiple filter
drop-shadowwere causing inner shadows inside the EuiPopover. Closes #6593.This PR is blocked by #6603.
Screenshots from Safari
Tried to recreate old shadow with only one
drop-shadowNew example added
This new example represents the data view picker in UnifiedSearch (elastic/kibana#151123). The example can be found in http://localhost:8030/#/navigation/context-menu#displaying-custom-elements
QA
How to test? Compare an old example with the new one in Safari:
Click Composers:
Click on Tag:
Test the new example that represents the use case from UnifiedSearch:
General checklist
- [ ] Checked in mobile- [ ] Props have proper autodocs (using@defaultif default values are missing) and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Added or updated jest and cypress tests- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] Updated the Figma library counterpart