rt icon indicating copy to clipboard operation
rt copied to clipboard

Move a UI pattern making a Submit button into a "full width" item into /Elements/Submit

Open obra opened this issue 1 year ago • 4 comments

This moves a bunch of style markup into a common location and should not result in noticeable rendering changes.

obra avatar Sep 07 '22 03:09 obra

I found three places that have a special case where the buttons were right next to each other and the update adds extra divs between, creating some extra space:

share/html/Admin/Tools/GnuPG.html share/html/Search/Build.html share/html/Ticket/Forward.html

Easiest might be to not use the new template for these?

Screen Shot 2022-09-08 at 11 53 53 AM Screen Shot 2022-09-08 at 11 53 41 AM

A few files report some stray whitespace (this is from git am with a patch from the branch):

Applying: Add a new 'FullWidth' option to /Element/Submit .git/rebase-apply/patch:15: trailing whitespace. % } .git/rebase-apply/patch:24: trailing whitespace. % if ($FullWidth) { .git/rebase-apply/patch:26: trailing whitespace. % } warning: 3 lines add whitespace errors. Applying: Move a UI pattern making a Submit button into a "full width" item into /Elements/Submit .git/rebase-apply/patch:127: trailing whitespace.

.git/rebase-apply/patch:551: trailing whitespace. Label => loc("Update"), .git/rebase-apply/patch:1069: trailing whitespace.

.git/rebase-apply/patch:2018: trailing whitespace.

warning: 4 lines add whitespace errors.

cbrandtbuffalo avatar Sep 08 '22 15:09 cbrandtbuffalo

I'll get the trailing whitespace cleaned up.

I'd argue that the extra padding around those stacked buttons is an improvement, as it makes it a little bit less likely to accidentally mis-click and improves tapablity on touch devices, but if it's breaking the current house style guide, I can revert that change in those three places.

On Thu, Sep 8, 2022 at 8:57 AM Jim Brandt @.***> wrote:

I found three places that have a special case where the buttons were right next to each other and the update adds extra divs between, creating some extra space:

share/html/Admin/Tools/GnuPG.html share/html/Search/Build.html share/html/Ticket/Forward.html

Easiest might be to not use the new template for these?

[image: Screen Shot 2022-09-08 at 11 53 53 AM] https://user-images.githubusercontent.com/1554082/189168632-ca7fd276-3300-4165-8842-a2ab99256cf3.png [image: Screen Shot 2022-09-08 at 11 53 41 AM] https://user-images.githubusercontent.com/1554082/189168636-8a7c0e87-015b-4853-b03b-0b1a9cdb2e55.png

A few files report some stray whitespace (this is from git am with a patch from the branch):

Applying: Add a new 'FullWidth' option to /Element/Submit .git/rebase-apply/patch:15: trailing whitespace. % } .git/rebase-apply/patch:24: trailing whitespace. % if ($FullWidth) { .git/rebase-apply/patch:26: trailing whitespace. % } warning: 3 lines add whitespace errors. Applying: Move a UI pattern making a Submit button into a "full width" item into /Elements/Submit .git/rebase-apply/patch:127: trailing whitespace.

.git/rebase-apply/patch:551: trailing whitespace. Label => loc("Update"), .git/rebase-apply/patch:1069: trailing whitespace.

.git/rebase-apply/patch:2018: trailing whitespace.

warning: 4 lines add whitespace errors.

— Reply to this email directly, view it on GitHub https://github.com/bestpractical/rt/pull/343#issuecomment-1240912165, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALC2FGCZVM3WYRRZF4J6LV5IEIJANCNFSM6AAAAAAQGLOLYM . You are receiving this because you authored the thread.Message ID: @.***>

obra avatar Sep 08 '22 18:09 obra

I'd argue that the extra padding around those stacked buttons is an improvement, as it makes it a little bit less likely to accidentally mis-click and improves tapablity on touch devices, but if it's breaking the current house style guide, I can revert that change in those three places.

We don't have an official style guide and stacked buttons don't come up much, so there aren't a ton of examples. To me it looks off because the spacing between the buttons is different from the space between the top button and the block above it. The Search button seems to be floating a bit and not completely grouped with the button above it. But Bootstrap added a bunch of spacing everywhere, so I've been sensitive to white space and trying to keep things tighter than the typical bootstrap defaults.

cbrandtbuffalo avatar Sep 09 '22 12:09 cbrandtbuffalo

Digging into the CSS, it looks like that unbalanced height was caused by this bit of css:

form div.submit div.buttons { min-height: 2.8em; }

That's legacy styling that predates the move to bootstrap. I'm going to commit a proposed removal of it.

(It's from 2010 and was implemented for the back/next feature in the installer UI which appears to work just great without it.)

[image: image.png]

commit 8cf8dcc309280a276624d82928a4c633c1258bce Author: sunnavy @.***> Date: Thu Dec 16 18:12:17 2010 +0800

make "back button" after "next button" so we can submit the "next

button" when hit enter

On Fri, Sep 9, 2022 at 5:35 AM Jim Brandt @.***> wrote:

I'd argue that the extra padding around those stacked buttons is an improvement, as it makes it a little bit less likely to accidentally mis-click and improves tapablity on touch devices, but if it's breaking the current house style guide, I can revert that change in those three places.

We don't have an official style guide and stacked buttons don't come up much, so there aren't a ton of examples. To me it looks off because the spacing between the buttons is different from the space between the top button and the block above it. The Search button seems to be floating a bit and not completely grouped with the button above it. But Bootstrap added a bunch of spacing everywhere, so I've been sensitive to white space and trying to keep things tighter than the typical bootstrap defaults.

— Reply to this email directly, view it on GitHub https://github.com/bestpractical/rt/pull/343#issuecomment-1241923658, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALC2GH5XASP5AD6ZWF3WTV5MVILANCNFSM6AAAAAAQGLOLYM . You are receiving this because you authored the thread.Message ID: @.***>

obra avatar Sep 13 '22 23:09 obra

@cbrandtbuffalo What else does this need before it's mergeable?

obra avatar Sep 29 '22 04:09 obra

The newest update makes sense, I just need some time to look again. Should be able to next week

cbrandtbuffalo avatar Sep 30 '22 15:09 cbrandtbuffalo

I squashed the trailing space cleanup commit, also tweaked a few more space changes and then merged.

sunnavy avatar Oct 07 '22 08:10 sunnavy