Bartik: fix button layout appearance on node add/edit form (small devices)

Can you provide more info ? I can't replicate the bug, this is how it looks to me:

Not sure if this is the best way to do it but here my first attempt

patch attached.

The fix in #5 does improve the display of the of the preview button, but if you're editing a pre-existing node, there's a delete button as well which doesn't sit nicely (see screenshot).

Also, rather than using an ID for the CSS selector, I'd use a class, perhaps something like:

.form-actions .button,
.form-actions input { ... }

I'm not that well up on Drupal coding standards though so someone else might be better placed to help you on the specifics!

First I tried using classes instead of the ID but then the .form-actions class could appear in a different non-related page, so I used the ID to force those buttons to look ok on that specific page.

But true, we can use classes and after do manual tests to make sure to it looks ok everywhere.

Hi,
I have created a patch to solve the issue mentioned at #6.

Below is the screenshot After the patch applied

Great work!

Some nitpicks in the last patch.

1. +++ b/core/themes/bartik/css/components/form.css
   @@ -262,7 +262,7 @@ input.form-submit:focus {
      margin-bottom: 0.4em;
   -}
   +}`
    #edit-actions input {
      margin-right: 0.6em; /* LTR */
   

   We need to remove this change.

2. +++ b/core/themes/bartik/css/components/form.css
   @@ -280,3 +280,24 @@ input.form-submit:focus {
   +}
   \ No newline at end of file
    
   

   All the files needs to have an extra new line in the end of the file.

Also this is going to need manual testing to make sure to this is not broke something else.

Thanks!

Hi,

Based on @gnuget updates and @Sakthi M last passed patch, I have created a patch. This is done against 8.2.x and 8.1.x already.

PS: As per @gnuget this still needs manual testing.

Local testing has the following output for me, which confirms fix.

@gnuget @johnrosswvsu Thanks!

+++ b/core/themes/bartik/css/components/form.css
@@ -280,3 +280,24 @@ input.form-submit:focus {
+    margin-left: 0;
+    margin-right: 0;
+    margin-top: 10px;

I think we can merge these 3 lines in one.

Hi,
gnuget It's a good suggestion and i have recreated the patch at #8. Good work @Sakthivel M.

 .form-item--error-message {
   color: #e32700;
 }
+
+/**
+ * Improve form element usability on narrow devices.
+ */
+@media all and (max-width: 600px) {
+  #edit-actions input#edit-preview {
+    float: none;
+    margin: 10px 0 0 0;
+    padding-bottom: 6px;
+    width: 100%;
+  }
+  .button--danger {
+    display: block;
+    margin: 10px 0;
+  }
+  .js .dropbutton .dropbutton-action > input, .js .dropbutton .dropbutton-action > a, .js .dropbutton .dropbutton-action > button {
+    text-align: center;
+  }
+}

Verified the patch ' bartik_form_button_appearance-2643206-15.patch' on drupal 8.0.x site
Steps performed are as follows
1.Installed Drupal 8.0.x and reproduced the issue
2.Applied the patch bartik_form_button_appearance-2643206-15.patch
3.Cleared cached and verified if buttons are aligned properly, but the changes are not reflected for delete button.
Attaching snapshot for reference.

I have recreated the patch for delete button #16

After Patch

The code must not contain IDs as this is going against the Drupal CSS architecture coding standards... https://www.drupal.org/coding-standards/css/architecture#avoid-structure

Use classes to assign appearance to markup. Never use id selectors in CSS.

This has already been pointed out in #7.

Please study the coding standards for Drupal Core carefully and try another fix.

Thanks.

Modified the patch in accordance with comment #18
Also made some changes as the style was not getting applied on anchor tags of other pages.

Also, the delete link doesn't look like a button for a reason this is on purpose because this makes the user to differentiate between the other actions and this one.

I'm not sure if we should just change the appearance of the button.

What do you think emma.maria ?

This also happens if the actions are inside a wrapper that is too small.
It's possible to do it depending on the size of the parent element instead of the size of the screen?
Then it would work in both cases.

@gnuget in bartik administration theme the delete does look like a button
The only thing we changed was increase width and add margin

Preview button appears to be fine. Save and publish button needs to be centre aligned

I have checked this issue, and have applied patch as mentioned in comment #19, its working fine. I am attaching screenshot for reference.

To check this, i made 'bartik' theme as an admin theme for node add/edit page too, then tested.

@soumyajit.basu Save and publish button working perfectly when administration theme is bartik. maybe you are choosing your administration theme is seven at the time Save and publish button text does not come at center.

So the issue is that centering the text in the dropdown button looks a bit off center.

So my solution is to add left padding 2em, which is the size of the dropdown toggle on the right side.

Attached is the patch for this, otherwise the button seems to look fine.

I have checked this issue with patch mentioned in comment #19 and its working fine. I think we should close this issue.

I'm at MidCamp Drupal Sprint, and I'm looking at this.

Just tested the patch on Simplytest.me running 8.0.5. Can also confirm patch at #31 works properly.

Cleaning up drupalcampfi tags.

I am assigning this to myself so I can take a look at this before this goes anywhere near RTBC. This needs at code review and more manual testing than just the specific component you are looking at here.

I also understand that Seven has exactly the same issue here #2643210: Seven theme: Fix text alignment on form submit buttons and we should also compare the work between issues.

Hi I have a suggestion on the presentation of the delete button. I have tried to revert the colors. so that it will be more deletious :)
I am attaching the Mock up

The design of the buttons themselves are out of scope for this issue.

However, we can raise a brand new issue for the theming of the Primary (eg. Save) and Danger (eg. Delete) buttons in Bartik.

@syamnath_zyxware please have a read of these issues about the design of components in the Seven theme and button styles specifically.
Proposal: A Style Guide for Seven
#2278715: Introduce a primary-danger link/button class
#1986074: Buttons style update

Hi
@Sakthivel M,
@johnrosswvsu,
@Anishnirmal,
@therealssj,
@Risse

When you create a patch can you please please *please* also create and upload an interdiff file.

It shows the differences between your patch and the previous one so a reviewer can clearly see the changes and decisions made between patches.
https://www.drupal.org/documentation/git/interdiff

Thanks!

The css was so small and had a simple change that's why i skipped interdiff.
Will keep that in mind in any future patches.

Why this closed without any discussion?
The patch also hasn't been merged yet!

Unassigned this from Emma Maria at DrupalCon New Orleans and asked for an interdiff file between the two most recent patches.

Thanks @stpaultim!

Hey I'm at Drupalcon New Orleans and I'll work on the interdiff for you.

Im at Drupalcon New Orleans and Im looking at this.

Worked on this with @aofficer. Attached is an interdiff of #31 and #19.

This patch scales and rounds corners of the save button.

Attached is an interdiff of #31 and #51.

I'm reviewing this patch now.

This is a HUGE improvement over the unpatched version. The stack with the delete button looks good in desktop and mobile, but without the delete button the rounding is off on mobile. Two screenshots attached:

With delete, looking snazzy

Without delete, looking almost snazzy

This patch handles one pixel issue.

There's now a 1px in the other direction, but I'm marking this as RTBC and we can open another issue for the small, teeny, TINY offset.

The test passed. Previous failures a false negative possibly due to Migrate.

This patch should address #60 and issues that may arise from different radius units. Rather than apply the radius to multiple elements, this adjusts the position of the parent so that the absolute positioned child (.dropbutton-toggle) honors the overflow:hidden of its container.

After, with delete:

If the ticket is still open for a pixel nitpick, the grey semi-circle is still a pixel too-high, but I'd mantain it's best to open a second issue for that and get this one merged in.

I'm going to take a look at the latest patch and review the CSS against the Drupal coding before handing it to @Cottser to assess for commit :-)

Hi,

Tested the functionality and it is working as expected the changes are correct on multiple browsers and mobile devices.

Not moving the status as @emma.maria will review the code.
(Reviewing patch first time!)

Code looks good to me. I have done some manual testing, here are some screenshots that i captured in narrow device.

But i have some confusion about the desktop view. There are irrelevant margin in delete button as compared to preview button.
I have already checked the issue description but Is it the part of bartik design ?

@Manjit.Singh : Can you please suggest me the proposed solution. Is it a part of bartik.? or needs to fix the margin for the delete button.?

I reviewed this patch. It fixes the original issues, and I can confirm that the spacing issue brought up in #70 was not introduced by this patch - it exists on a bare install of Drupal 8.2. If we think this is a major problem, a new issue should be created for fixing the spacing.

@starshaped Agree with the thoughts.
Thanks.

Thanks, all! Assigning to @Cottser as per above.

This should make it into 8.2.x and can go in during beta. If not it can still go into 8.3.x.

There are definitely some flaws with dropbuttons in Bartik but that's outside of the scope here and I think at least one issue exists already. (Background color of the arrow when hovering, and the arrow background not filling up the entire space.)

This is a big improvement and code-wise looks good. Removing the manual testing tag since that did happen (and I just tested as well).

Committed d9a6637 and pushed to 8.2.x. Thanks!