fix(core/dropdown): adjusted height for dropdown (siemens#1671) #1762
Conversation
|
|
There was a problem hiding this comment.
Thanks for your contribution, looks like your screenshots for testing are generated with windows. You have to generate it via a linux based system: https://github.com/siemens/ix/blob/main/CONTRIBUTING.md#visual-regression-testing
| @@ -33,7 +33,7 @@ | |||
| } | |||
|
|
|||
| :host(.overflow) { | |||
| max-height: 50vh; | |||
| max-height: calc(50vh - 50px); | |||
There was a problem hiding this comment.
Maybe we can use here a variable which makes it looks less like a magic number 50px
There was a problem hiding this comment.
Just thought myself is a visual-regression test here really necessary? Should be also possible via a simple component test which makes snapshot creation not necessary.
There was a problem hiding this comment.
Additional/edge cases (e.g. dropdown opened via trigger element, dropdown opening above trigger) are missing.
| @@ -33,7 +33,7 @@ | |||
| } | |||
|
|
|||
| :host(.overflow) { | |||
| max-height: 50vh; | |||
| max-height: calc(50vh - 50px); | |||
There was a problem hiding this comment.
Rem should be used over px.
There was a problem hiding this comment.
Additional/edge cases (e.g. dropdown opened via trigger element, dropdown opening above trigger) are missing.
💡 What is the current behavior?
GitHub Issue Number: #1671
🆕 What is the new behavior?
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm run docs)pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support