-
Notifications
You must be signed in to change notification settings - Fork 1
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
Docs(web-twig): Demonstrate composition of Header
with Dropdown
#1048
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for spirit-design-system-storybook canceled.
|
✅ Deploy Preview for spirit-design-system-demo canceled.
|
✅ Deploy Preview for spirit-design-system-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for spirit-design-system-validations canceled.
|
...s/web-twig/src/Resources/components/Header/stories/HeaderInvertedWithActionsAndDropdown.twig
Outdated
Show resolved
Hide resolved
👏 Great. But consider usage of the button instead of the link for dropdown trigger :-) |
...s/web-twig/src/Resources/components/Header/stories/HeaderInvertedWithActionsAndDropdown.twig
Outdated
Show resolved
Hide resolved
...s/web-twig/src/Resources/components/Header/stories/HeaderInvertedWithActionsAndDropdown.twig
Outdated
Show resolved
Hide resolved
|
||
<HeaderDesktopActions color="secondary"> | ||
<HeaderNav> | ||
<HeaderNavItem UNSAFE_style="position: relative;"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a foolish idea, but what about to apply display: contents;
to the DropdownWrapper
?
Then we could also use this component in the cases like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, it's not true. It would affect the Dropdown menu. 🙈
But hypothetically flex
… 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
display: contents
would make DropdownWrapper
disappear from DOM which would lead to a misplaced Dropdown
…
Originally, I was even considering rather a helper class like position-relative
because that's the only thing I want it to do. Anything else could potentially cause more harm than good, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a high chance that with Breeze version we are able to solve this case without additional styles – @adamkudrna will take a look at it if there will be a task in sprint. 🙌🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please fix the already mentioned problems.
Also please add this demo to the React and Web packages.
03d27f1
to
8aaf900
Compare
Changes unknown |
8aaf900
to
d4c3c47
Compare
d4c3c47
to
da1e1f8
Compare
☁️ Nx Cloud ReportCI is running/has finished running commands for commit da1e1f8. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 3 targetsSent with 💌 from NxCloud. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
✅ Deploy Preview for spirit-design-system ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
d952b0f
to
ba53f06
Compare
☁️ Nx Cloud ReportCI is running/has finished running commands for commit ba53f06. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 3 targetsSent with 💌 from NxCloud. |
Description
Demonstrate the composition of
Header
andDropdown
.Additional context
Issue reference
N/A