Skip to content
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

Basis: Needs styling on Node links like "Read more" and "Comments" #2797

Open
quicksketch opened this issue Aug 17, 2017 · 45 comments · May be fixed by backdrop/backdrop#4967
Open

Basis: Needs styling on Node links like "Read more" and "Comments" #2797

quicksketch opened this issue Aug 17, 2017 · 45 comments · May be fixed by backdrop/backdrop#4967

Comments

@quicksketch
Copy link
Member

quicksketch commented Aug 17, 2017

Describe your issue or idea

The styling on the node links could use some work in Basis. Right now they stack on top of each other, rather than displaying one after the other:

image

Steps to reproduce (if reporting a bug)

  • Install Backdrop.
  • Enable comments on the first post.
  • Add comments to the first post.
  • View the homepage.

Recommended solution
Remove the display: block style on .links.inline .node-readmore in basis/css/component/small-text-components.css

Status

Blocked until we find a better way to make backward compatible CSS Changes.
#4167

@ghost
Copy link

ghost commented Jun 5, 2022

Confirmed; still an issue in the latest version:

IMG_20220605_120240

@ghost
Copy link

ghost commented Jul 3, 2022

The PR was simple enough: backdrop/backdrop#4122

The implications of it, on the other hand, might be trickier to address...

@ghost
Copy link

ghost commented Jul 3, 2022

image

@argiepiano
Copy link

argiepiano commented Jul 3, 2022

I find the comma separator in "Read more, 1 comment, Add comment" rather confusing. The commas make them seem more semantically related than they are (not to mention that "Add" should not be capitalized after a comma, but this is not this PR's fault). Specifically "Read more, 1 comment" makes it seem like the Read more is somehow related to comments, but it's not.

@bugfolder
Copy link

For parallelness of construction, grammar, and punctuation, should it be

Read more; see 1 comment; add comment

?

@bugfolder
Copy link

I added another PR that fixes the capitalization of "Add" in addition to the CSS change of the existing PR.

image

There's still the question of whether inline links should be delimited with something "stronger" than commas (like semicolons, in my comment just above).

Even though semicolons are more grammatical in this example, since inline links are used in a lot of places, it's not clear that they should be replaced everywhere. If it's a good idea for here, then maybe should we support another CSS class that replaces the commas with semicolons in specific locations?

@bugfolder
Copy link

Meanwhile, tests need to be updated for this PR (to check for consistent capitalization).

@olafgrabienski
Copy link

olafgrabienski commented Oct 1, 2023

I added another PR that fixes the capitalization of "Add" in addition to the CSS change of the existing PR.

The fix works for a post with comments (screenshot No. 1), but not for content without comments (No. 2).

No. 1, Post with comments

No. 2, Post without comments

@olafgrabienski
Copy link

olafgrabienski commented Oct 1, 2023

There's still the question of whether inline links should be delimited with something "stronger" than commas (like semicolons, in my comment just above).

I agree, commas aren't the best option here, but I don't like semicolons either. In my opinion, both are misleading, suggesting that inline links are part of a grammatical sentence. I see them rather as independent elements. To make this more clear, they could be separated by an element which is normally not used grammatically (not as punctuation in the strict sense), and to keep the "Add" capitalized. Ideas:

Slash:
Read more / 1 comment / Add comment

Pipe:
Read more | 1 comment | Add comment

Middot:
Read more · 1 comment · Add comment

Spacing (CSS margin):
Read more 1 comment Add comment

My favorite: middot, or spacing

@avpaderno
Copy link
Member

Commas are used for lists, but Read more, 1 comment, Add comment is not a list; it could be a list, if it listed actions to do on comments.

I prefer middots as separators; spacing is my second choice.

@olafgrabienski
Copy link

Read more, 1 comment, Add comment (...) could be a list, if it listed actions to do on comments.

Interesting! I guess, that was the reason for @bugfolder's addition of a verb ("see comment") here: #2797 (comment)

@bugfolder
Copy link

Middot:
Read more · 1 comment · Add comment

Middot is my favorite, too, and a nice thing about not using sentence punctuation is then capitalizing each element (current behavior) makes grammatical sense.

that was the reason for @bugfolder's addition of a verb ("see comment") here

Yes, that way they're all imperative sentences.

@bugfolder
Copy link

I modified the PR to go back to always capitalizing and use heavy dots for delimiters (which seemed better balanced to me):

image

Using diferent delimiters for inline links seems to makes sense here, but what about generally?

I did a search in core for 'links', 'inline' (to find instances of '#attributes' => array('class' => array('links', 'inline')), and see examples in:

  • book.module
  • comment.entity.inc
  • comment.module
  • file.entity.inc
  • node.entity.inc
  • redirect_handler-field_redirect_operations.inc

As for changing N comments to See N comments for links, I'd like to hear more of others' thoughts on that.

@olafgrabienski
Copy link

Using diferent delimiters for inline links seems to makes sense here, but what about generally?

I'd say it makes generally sense. It would be good to check with some examples, but I don't have the time at the moment.

Re changing N comments to See N comments, I prefer the first (less noisy). Actually, I think it deserves a separate issue.

@findlabnet
Copy link

I think things like different delimiters are a matter of user taste or styling rules, so it should be configurable just like breadcrumbs delimiters in some themes.

@bugfolder
Copy link

Here's an example using redirect_handler_field_redirect_operations.inc: create a View table of URL redirects, then add the Operations field. In Seven (in the View editor), it looks like this:

image

And in Basis, with the dot delimiters, it looks like this:

image

I don't think it needs the dots here, but they don't hurt. (Actually, I'd prefer that the "Operations" render as dropbuttons, as is done in so many other places for multiple operations, but that would be a different issue.)

@bugfolder
Copy link

For the usage in node.entity, this theming is used for the "Read more" link in teasers, but since there's only one link, there's no delimiter. But modules could (via hook_node_view or hook_entity_view add additional links.)

@bugfolder
Copy link

For the usage in file.entity, the links are themed inline, but the list of links is empty unless augmented via hook_file_view or hook_entity_view.

@olafgrabienski
Copy link

@bugfolder Thanks for the examples! Looks like your statement about one of them (dots not needed, but they don't hurt) is valid for most places. However, with different ideas like dropbuttons under consideration, I'm not sure if it's worth the effort to change all the places now.

@bugfolder
Copy link

I'm not sure if it's worth the effort to change all the places now.

If we change the CSS that affects the read more/comments display, it will automatically affect all the other places, so no additional effort is needed to change those. I think the reason for checking the other places was to make sure that this change didn't adversely affect other places. (And thus far, I don't think it does.)

I'd suggest we go ahead and make the CSS change to dots. It will improve the display of links for read more/comments, won't hurt anywhere else. And then as for the other things discussed:

  • Changing redirect operations field in Views to dropbuttons could be a new issue (and then it wouldn't be affected by this change).
  • Letting users change the delimiter from dots to something else is easily accomplished by subtheming Basis, and that avoids adding another setting to Basis; but if there was strong support for making that a Basis setting, it could then be another issue.

How's that sound?

@bugfolder
Copy link

So you can say that that's an existing pattern we should retain I think.

I agree. So, to recap...before:

Updated PR:

image

@olafgrabienski
Copy link

olafgrabienski commented Oct 5, 2023

Hm, looks I was a bit slow and the PR is already updated: Personally I don't like the pipes, too obtrusive, and I like the dots much better.

Re consistency, the Views UI is a different thing, and it wasn't revised for a long time. We can follow its pattern, but in my opinion we don't have to.

@bugfolder
Copy link

I guess I don't have a strong opinion myself (though consistency, however tenuous, won the day for me here). Since we clearly have a difference of opinion, how shall we settle this? (Perhaps our designer, @dariusgarza, could chime in?)

@olafgrabienski
Copy link

Thanks @bugfolder, and thanks for all the work on this. In a way, I have a strong opinion, but it's also a personal preference, and I don't want to derail the issue. The pipe symbol is also fine for me.

@avpaderno
Copy link
Member

I find the pipes in the last screenshot fine too. I guess it may be because they seem like the borders of table cells.
In the case the part after the delimiter could be rendered in the next line, I find the pipes more appropriate.

English does not use middle dots as punctuation anymore, but some languages use them. This could be a reason for avoiding middle dots to separate links.

@klonos
Copy link
Member

klonos commented Oct 7, 2023

Speaking of existing patterns, we have used middots to indicate indentation/nesting in issues like #4770, #4218 etc.

@olafgrabienski
Copy link

Speaking of existing patterns, we have used middots to indicate indentation/nesting ...

Funny! When we introduced middots in #4218, it was a good choice because they didn't follow existing patterns ;-)

@bugfolder
Copy link

Seems like enough time and feedback has elapsed to to remove the "needs feedback." Looks like everyone thus far either prefers pipes or is at least OK with pipes, so I'll suggest we move on to the next phase of testing & code review. (Or express newly strong opposition, as the case may be.)

@stpaultim
Copy link
Member

I'm good with the middots or pipes. My concern is whether or not any changes to this might break websites that have already sub-themed basis to fix this problem in their own manner?

@stpaultim
Copy link
Member

stpaultim commented May 9, 2024

I brought this issue to the developer meeting today to ask if we thought that this was backward compatible. The general consensus was, that based upon other decisions we've been making, this would not be considered backward compatible at this time.

So, I think the official status of this issue would be on hold until we come up with a way to make backward compatible css changes. See: Supplemental CSS Selectors (or one of the other ideas out there). #4782

#4167 - Decide on best way to make improvements to the CSS for core without breaking existing sites

@stpaultim
Copy link
Member

We now have the ability to fix this in Basis.

@bugfolder are you able to rebase the PR? We have until Jan 15th to get this into the next release.

@bugfolder
Copy link

PR is rebased (new PR), and RTL support is added.

LTR:
image

RTL:
image

@laryn
Copy link
Contributor

laryn commented Jan 2, 2025

I posted a comment in Zulip asking about whether we can/should use logical properties instead of hard-coding RTL separately, going forward.

@bugfolder
Copy link

Here's the corresponding logical properties issue for Backdrop core: #6657

Based on the Zulip discussion, making the switch to logical properties in core may take a while; I suggest that it need not hold up current PRs that contain LTR/RTL CSS (such as this one), so I'll suggest/request continued review/testing of the current PR.

@laryn
Copy link
Contributor

laryn commented Jan 3, 2025

Agreed.

@stpaultim
Copy link
Member

It seems that this issue was blocked by the backwards compatibility issue. So, now it needs the update-1-30 css class to ensure backward compatibility.

@bugfolder
Copy link

bugfolder commented Jan 3, 2025

now it needs the update-1-30 css class to ensure backward compatibility.

@stpaultim, can you explain further, or link to where in the docs this class is explained? (I'm not seeing it on the docs site, except in test code.)

EDIT: OK, it's here: #4782

(Off-topic, but it would seem some discussion of update classes could benefit https://docs.backdropcms.org/css-standards.)

@bugfolder
Copy link

update-1-30 class added to CSS selectors. Results:

  • No update class on body, LTR:
    image

  • No update class on body, RTL:
    image

  • Add update-1-30 class to body, LTR:
    image

  • Add update-1-30 class to body, RTL:
    image

Ready again for review.

@stpaultim
Copy link
Member

@bugfolder - You are correct, that we probably need some documentation for contributors on how to use the new CSS classes. I made a note of that here:

backdrop-ops/docs.backdropcms.org#244

I'm waiting for feedback on the documentation that I already created.

Sorry, I had intended to provide some suggestions for you, but didn't get to it.

@stpaultim
Copy link
Member

stpaultim commented Jan 4, 2025

I like the pipes. Going to mark this WFM. It's been discussed a great deal already.

I also marked this as a candidate for the Bug Fix Milestone. Does someone want to add it to the Milestone?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment