The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- Status changed to Needs review
over 1 year ago 5:01am 31 January 2023 - 🇮🇳India Gauravvv Delhi, India
Fixed CCF, attached interdiff for same.
Also we're not using any variables in the file, so I have removed the import of variable file as well. - 🇮🇳India Nayana Ramakrishnan
Verified the patch #105 and tested it on Drupal version 10.1.x with Olivero theme. The patch works fine and I have added the before and after screenshots for reference.
- 🇺🇸United States smustgrave
#101 updated issue summary so removing that tag.
+1 For me as this does solve the issue at hand. Not moving as still need review from olivero maintainers.
- Issue was unassigned.
- Status changed to Needs work
over 1 year ago 5:32pm 23 February 2023 - 🇺🇸United States andy-blum Ohio, USA
This looks good, and I think would probably be fine, but the fact that we're targeting all ui-dialogs in Olivero is reason enough to make sure this is bulletproof. As much as the design looks better with the buttons in alignment, it's more important that we don't
.ui-dialog { & .ui-button { &:last-child { margin-inline-end: 0; } } }
This is targeting all buttons that are the last child in their wrapper, including the close button. This doesn't appear to change anything as that button doesn't have a margin, but since we're already digging into the buttonpane and buttonset wrappers later, we might as well limit the scope of this rule.
.ui-dialog { & .ui-dialog-buttonpane { padding-inline-start: 0.2em; padding-inline-end: 0.2em; & .ui-dialog-buttonset { display: flex; float: none; } } }
Since we're setting the buttonset to
display:flex;
, I think we ought to consider a few things:-
If we add
flex-wrap: wrap;
to the buttonset element, that will allow buttons to wrap to the second line if they need it, before wrapping extremely long text and overflowing the button's boundaries.
- If we space the buttons in the buttonpane using the
gap
property, we don't have to worry about margins on any hypothetical 3rd buttons.
-
If we add
- 🇮🇳India pradipmodh13 Ahmedabad
Hello @andy-blum,
Added flex-wrap: wrap in ui-dialog-buttonset for global implementation.
Please review.
- Status changed to Needs review
over 1 year ago 2:03pm 13 March 2023 - Status changed to Needs work
over 1 year ago 2:05pm 13 March 2023 - 🇺🇸United States andy-blum Ohio, USA
I know it's sandwiched between the images, but still needs point #2 from my earlier review:
If we space the buttons in the buttonpane using the gap property, we don't have to worry about margins on any hypothetical 3rd buttons.
- Status changed to Needs review
over 1 year ago 6:42am 14 March 2023 - 🇮🇳India pradipmodh13 Ahmedabad
Hello @andy-blum,
Added gap property for button layout to handle global implementation.
Please review.
- Status changed to RTBC
over 1 year ago 1:41pm 14 March 2023 - 🇺🇸United States andy-blum Ohio, USA
This looks good to me. Thanks @pradipmodh13
The last submitted patch, 113: 3149863-113.patch, failed testing. View results →
- Status changed to Fixed
over 1 year ago 11:26am 17 March 2023 - 🇳🇿New Zealand quietone New Zealand
It looks like manual testing was done in #144. I am removing the tag.
Automatically closed - issue fixed for 2 weeks with no activity.