Inconsistent use of styles on Flexible Template Builder UI

[jenlampton]

Description of the bug

The UI for building a Flexible Template has some unusual CSS patterns that are not consistent with the rest of core. This page should have similar design patterns to the Manage Blocks page. That said, the elements we are moving around are not blocks, so they should not match the pattern for blocks on the manage block page to avoid confusion.

Some individual problems are noted below, but this page could probably use a a design review / refresh as a whole.

• Strange spacing between border and interior
• use of blue dotted line (unique to only this page)
• box shadow too large (should be box-shadow: 2px 2px 0 0 #d0d0d0; for consistency)
• combination of box-shadow and dotted line looks strange

[stpaultim]

I took a stab at making some improvements here. Sure, we could always use a designer input, but is this good enough (with feedback) to move forward until we get a designer. (Actually, I did get input on most of the changes I made today from a designer at Twin Cities Drupal January sprint).

Before:

After (CURRENT PR):

For reference, this is the current block layout page (manage blocks on layout):

I welcome thoughts and ideas for improvement.

Atten: @jenlampton

[yorkshire-pudding]

@stpaultim - it is a definite improvement. I don't have any suggestions for improvement and I would be happy with this.

[stpaultim]

@yorkshire-pudding I put this on the agenda for the design/ux meeting next week.

I'm open to suggestions for how to make this revision better OR we could work incrementally and make more changes later.

[yorkshire-pudding]

@yorkshire-pudding I put this on the agenda for the design/ux meeting next week.

I'm open to suggestions for how to make this revision better OR we could work incrementally and make more changes later.

Unless there are concrete suggestions, I'm a strong fan of incremental improvement

[klonos]

Some feedback from today's dev meeting:

1. looks good and is definitely an improvement 👍🏼
2. the cursor on-hover over the rows should be a "vertical move" cursor (up/down) - while the current cursor is a "move cursor" (all directions, which is wrong for the flexible layouts UI, as the rows can only be moved up/down), and the PR changes it to a default "select text" cursor (which is a regression).
3. Related to the above, the "move" icon (the one to the left of the row name) will also need to change (but it seems that we will need to introduce a new icon for that). Related: #5152
4. some attention in the left/right/up margins of the row headers (they should be removed, so that the row "attaches" to the border of the container).
5. The font size of the row header text is smaller than the respective used in the blocks layout - needs to be adjusted.

(I hope I captured everything)

[stpaultim]

PR is about to get updated:

I changed the cursor back to the one we were using before (fixed accidental regression). I did not do an up/down cursor, because I did not see one in standard CSS and it seems we have not yet implemented #5152

In regards to @klonos item 5) My inspection shows same size fonts for header on Blocks Layouts and on Flexible Layouts Template configuration. 1.1em or 17.6px

[stpaultim]

Incorporated feedback from @jenlampton . Thanks so much for taking a look. I appreciate it.

[klonos]

@stpaultim #5152 is in order to change the drag icon (the one to the left of the row titles) to a .svg provided by the Phosphor icon set in core (point 3 in my previous comment in this thread), whereas what needs to be changed to up/down is the standard mouse cursor CSS property to be cursor: row-resize; (which is point 2 in my previous comment). See https://www.w3schools.com/cssref/pr_class_cursor.php and https://www.w3schools.com/cssref/playit.php?filename=playcss_cursor&preval=row-resize specifically.

One thing that has been mentioned before is the lack of sufficient contrast with the various shades of gray and white we use, but we have #1632 for that. What we could do here though is to add some border to the regions. If we go with the same dashed border as in the "Manage blocks" UI (border: 1px dashed #CCC; instead of the current border: 1px solid white;), then it would suffice and be consistent I believe.

Other than those issues, I think that this good to go. Thanks @stpaultim 🙏🏼

[klonos]

If we go with the same dashed border as in the "Manage blocks" UI ...

...actually, simply removing the following fixes that (I've added a comment in the PR):

.l-flexible-row .layout-editor-region {
  border: 1px solid white;
}

[stpaultim]

standard mouse cursor CSS property to be cursor: row-resize;

@klonos I looked for a standard cursor that did this and did not find this one on my own. Thanks for pointing this out. Change made, along with your other suggestions.

Does anyone else have any thoughts on this PR?

[jenlampton]

@klonos @stpaultim As soon as we get rid of the dotted lines on the regions, I think this is RTBC!

The dotted lines mean "you can drop things here" and since we are dragging rows on this page (and not blocks) and since rows cannot be dropped into regions, the dots need to go. (This was discussed very early on, I think, you may have missed that part of the discssion @klonos.)

[jenlampton]

We could be a little more consistent with the manage blocks interface if we made the main background gray, and also provided a drop-zone for rows with a white background and gray dashed border.

Manage blocks page with drop zone:

Flexible layout page with drop-zone:

I'll make a PR against your PR @stpaultim

[jenlampton]

@stpaultim I'm not sure I like this any better, so I provided an alternate PR (instead of one against yours) where people can see what the page would look like with a drop-zone for rows: https://github.com/backdrop/backdrop/pull/4738

Here's the page with an additional drop-zone for rows added (more consistent with manage blocks)

I think I prefer the current PR but without the drop-zone for blocks:

@klonos would love your opinion here too :)

[stpaultim]

In terms of consistency, I think that there is enough of a difference between the manage blocks and flexible layouts page that consistency is at least somewhat in the eye of the beholder. It's not clear to me that either approach is objectively more consitent?

Having said that, I think I would vote for the second one. BUT, I'd be fine with either one.

@Klonos asked me to put the dotted line around the regions to be more consistent with the manage blocks page. Then you removed it. I'm not sure why or if that was intentional. On the manage layouts page, regions have a dotted/dashed border. I had added that to my PR for consistency with flexible layout page, but your PR against my PR removed it again. Was that intentional?

@jenlampton

[stpaultim]

At @jenlampton My last version had this dashed line (actually, the dashed line is inherited from existing css, I had overridden it with a solid line, but I removed the override in my last draft at the suggestion from @klonos).

The manage blocks page puts this around regions as well.

Your PR against my PR overrides that dashed line again and makes it solid again.

[klonos]

The dotted lines mean "you can drop things here" and since we are dragging rows on this page (and not blocks) and since rows cannot be dropped into regions, the dots need to go. (This was discussed very early on, I think, you may have missed that part of the discssion @klonos.)

Gotcha 👍🏼 ...makes sense. I didn't know that that was the convention, and yes, I missed the discussion about it. Sorry.

Here's the page with an additional drop-zone for rows added ... @klonos would love your opinion here too :)

Yeah, I don't think that dropzones in the template UI makes sense in the way that region dropzones make sense in the blocks UI.

@klonos asked me to put the dotted line around the regions to be more consistent with the manage blocks page. Then you removed it. I'm not sure why or if that was intentional. ...

Apologies for the confusion @stpaultim ...I did miss the previous discussions, and what @jenlampton is describing makes sense. My thought process when I made the suggestion I made was more like this: I noticed that there is some rather serious contrast issue in the tones of white/gray we are using around the regions and their row container -> I then opened another tab with the blocks UI in order to compare and see why this issue was not as prominent to my eyes before -> then saw that the regions in the block UI have the dotted border, which made things a bit better -> then tested the same in my local with the template UI and it seemed like an improvement. Now that I know that there is a mental association with the dotted border as a dropzone, I realize that what Jen is saying makes sense. ...so if I'm not asking too much, can we then please try changing the dotted line to a solid one, but with the same gray color (#ccc I believe)? (instead of the solid white like you had it in your original PR, which made no difference anyway)

[klonos]

...can we then please try changing the dotted line to a solid one ...

Never mind, I see that that's been done in the latest commits. Re-reviewing 👀

[stpaultim]

So, I think my version of the PR has all the requested changes and the agreed upon version of the drop zone (or lack thereof).

@klonos and @jenlampton - What next?

[klonos]

@stpaultim I've filed a PR against your PR: https://github.com/stpaultim/backdrop/pull/7

Before you merge it, I'd like @jenlampton's and other people's 👀 on it please. See comments and screenshots in the PR summary.

[stpaultim]

@klonos I agree that the contrast is not great pretty bad. I tried to keep my focus on consistency between the two pages and was reluctant to make this kind of improvement, for fear that scope creep (keeping two pages consistent, while making improvements) would bog down the issue.

Having said that. Your scope creep is ONLY 1-2 lines of css and only effects one other file. I think this increase in scope is fine (and preserves consistency) and I do think it makes the two pages easier to read or look at. So, I'm in favor of your change, if others are ok with it as well and as long as it does not open the door to a bunch of other new "improvement" requests.

Happy to file a follow-up issue to take the redesign even further at a later date.

[klonos]

Adding the screenshots for the PR here, so that people can nay or yay in order to move this issue on:

...adds consistent 1px solid #cccccc; in various places - see before(Tim's PR)/after(this PR) side-by-side screenshots:

• template UI:
• blocks UI:

[stpaultim]

STATUS: I believe that @jenlampton has agreed that we should stick with my PR. She experimented with some changes, that we decided against. This is a pretty simple PR that has been reviewed by both @klonos and @jenlampton and feedback has been incorproated.

I think the only hold up on this is the decision on whether or not to include the border change that @klonos describes in the last comment.

Any feedback on that border? (I like it)

I would argue that given that we're are talking subtle change to CSS for the admin theme, we could merge the improvements already included and futher interate later...

[klonos]

@stpaultim I'm happy to merge this as is and follow-up on the a11y and border line issues 👍🏼

@jenlampton since you were the one that set the label of this issue to "needs work", I'll leave it up to you to make the call. If you like the results of the before/after in the screenshots in my previous comment here, then it would be as simple as @stpaultim merging my PR against his branch. If not, then we can leave it for a follow-up.

[stpaultim]

@klonos, @jenlampton and I looked at this during the most recent dev meeting and agreed on the changes. Someone take a final look at it and mark it WFM!

Let's try to get this into the next bugfix release.

[klonos]

@stpaultim my concerns about the same border added consistently around the draggable/droppable items in both UIs (rows in the flexible template UI, blocks in the layout UI) has been addressed in the current PR 👍🏼 ...I've left a small comment about a CSS formatting issue (missing zeros from decimal values).

I believe that @jenlampton suggested that we wrap all rows around a dashed line ...not to add it to each row individually - just add another wrapper around the area where they can be dropped. This is the screenshot Jen provided with the desired outcome:

I agree with that suggested change, however the screenshot above is leaving the "Add row" button out of that dotted frame, whereas the respective "Add block" button is inside each such frame in the Blocks UI:

So the end result in the flexi template UI should ideally be:

• outer gray overall frame, with #f7f7f7 background and a 2px solid border of #eaeaea and a border radius of 4px (see #layout-edit-main)
• inner white overall frame with a 1px dashed border of #cccccc (see .layout-editor-region)
• the "Add row" button should be inside the white dashed frame (along with all the rows)
• I'm happy with the current style of the draggable/droppable rows and they seem to be consistent with the style of the draggable/droppable blocks in the Block UI 👍🏼

Setting the status to "needs work" for the above.

[stpaultim]

Thanks for review, I commited your suggested syntax change.

@klonos said:

I believe that @jenlampton suggested that we wrap all rows around a dashed line ...not to add it to each row individually - just add another wrapper around the area where they can be dropped.

@jenlampton suggested that and created a PR to do that, but then decided she preferred it without that extra layer. See ("current PR" was my PR without the extra drop-zone):

@jenlampton said: https://github.com/backdrop/backdrop-issues/issues/6325#issuecomment-2101698094

I think I prefer the current PR but without the drop-zone for blocks

I'm getting dizzy, going back and forth here. :-)

[stpaultim]

I've removed the "Needs Work" label and am asking for both Testing and Review once again.

This nice little change is very near to RTBC. I believe that the last concern by @klonos about the placement of dotted line has been discussed and resolved.

Does anyone else want to chime in on that?

[avpaderno]

I was going to take a look, but the preview sites for both the PRs expired.

[avpaderno]

I just noticed a behavior: When I add a new row, the row is shown after the footer, but when I try to drag the new row to place it before the footer, the footer suddenly appears as last row.

[avpaderno]

Furthermore, after I push the footer row to the bottom, and I click on Save layout template, when I click on the footer row, a row is still shown after the footer row.

I always click on Save layout template after I add a new row and move back the footer row to the bottom.

[avpaderno]

Even more "complex": I push the footer row to the bottom, I click on Save layout template. I click on the footer row to move it, and a row is shown after the footer row. I scroll the page up and down, I click again on the footer row to move it; this time two rows are shown after the footer row.

[stpaultim]

@avpaderno - I'm confused. To start with, have you looked at the code? This PR makes some pretty basic CSS changes to things like background-color and line weight. So, I don't think this PR could be doing anything to effect the placement of regions. Are the behaviors you are noting specific to this PR or simply things that work differently in Backdrop CMS than you would expect?

When I add a new row, the row is shown after the footer, but when I try to drag the new row to place it before the footer, the footer suddenly appears as last row.

Is this not the expected behavior? When you add a new row, it always go to the end of the layout and then you move it into place. There might be a better way for this to happen, but if so, that would be a new feature request.

If there is a region below the footer region and I drag it above the footer, then I would expect the footer to be on the bottom (to be the last row). Right?

Is all of your testing in the sandbox site? Have you compared behavior in this PR to behavior in a normal Backdrop site? Maybe I am misunderstanding something. But, I am unable to create any unusual or unexpected behavior around the placement of regions in the sandbox for this PR.

[avpaderno]

I apologize for the confusion: I tried the preview site for this PR and I commented on what I noticed. I did not mean it was caused by this PR.

Surely, there is JavaScript code that handles the drag-and-drop. I could not say what causes the behavior, but it happens in the moment I try to drag a row. I would say it is caused by the used JavaScript code, which could also not correctly change the used CSS classes.

(It is not about what I expect. I guess that everybody would be confused when they try to move a row on the bottom, and that row suddenly appears on the bottom of all the rows.)

[avpaderno]

I could reproduce it on https://pr4827-u4dgvncmnhsntqfqnubjrfn3j8qnxi7e.tugboatqa.com/admin/structure/layouts/settings/flexible-template/demo/configure too, and I noticed it happens even without dragging any row.

I will create an bug report about that.