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

Add support for "Relationships" to Layout Contexts. #2134

Closed
quicksketch opened this issue Sep 4, 2016 · 68 comments · Fixed by backdrop/backdrop#3611
Closed

Add support for "Relationships" to Layout Contexts. #2134

quicksketch opened this issue Sep 4, 2016 · 68 comments · Fixed by backdrop/backdrop#3611

Comments

@quicksketch
Copy link
Member

quicksketch commented Sep 4, 2016

In many situations when working with Layouts you want or need information related to the primary thing being displayed. For example if you had created a layout configuration for the path node/%, you have the "Node" context available but not the "User" context for the author of that content. This means that you can place the fields from the node as blocks, but not fields from the node author user account.

By adding relationship support, it would make it possible to display blocks (or use conditions) from the content being displayed but from any object in Backdrop, as long as it was related to the original piece of content in some way.

This has come up a few times in issues such as #1815 (Does it make sense to allow multiple contexts for a single layout?) plus a few more related questions (#1815) and in the original Layout module issue #86 as an item that needs to be done eventually, but we hadn't yet had an issue for it.

Update: How this all works, a summary:
Blocks display things, and often they need to display things from entities. For example you might have a layout with a "Node information block" in that layout. The code for that block could check which path you're on, and have a bit like if arg(1) == 'node' && arg(2) is a number etc etc then load that node with that NID to get the node details, and that's fine.

But Backdrop also arranges that a Layout can provide that info directly to the blocks code so the dev doesn't have to check if the block is on a node page. So your block code simply checks for does this layout have a node (something like that).

Layout would store that node info in a "container" called a context.

So Layouts with "node paths" like node/% automatically provide a node context to its blocks. Layouts with "user paths" like user/% provide a user. We recently made it so that you can manually create a new context container on your layout, with the "custom context" functionality.

But what if you want to show the author of the node on all node pages? There couldn't be a "user container" on that layout.

This PR then allows you to create context containers storing context that are related to other contexts already on the layout. So if your layout already has a node context, you can tell layout to automatically create another context with the author of that node, by adding a "User from node" relationship.

When you visit a page using that layout, say at a node/% path, and you've added a "User from node" relationship, in that layout there will then be two containers holding contexts, one with the node, another with the author of that node. Blocks that want node contexts would show up, and blocks that want a user context would also be happy.

Advocate: @docwilmot


PR by @docwilmot: backdrop/backdrop#1733
Replaced by PR by @docwilmot: backdrop/backdrop#3422
Replaced by backdrop/backdrop#3611 by @docwilmot and @quicksketch

@quicksketch
Copy link
Member Author

The way I had originally intended this to work was that Relationships would be specified on the Settings page of the layout configuration:

selection_179

A new section would be added for "Relationships" (if any contexts were available) allowing you to create relationships from the existing contexts to any other type of data you wanted to pull in. Once added, all the available blocks and conditions from that type of context would be available.

@quicksketch
Copy link
Member Author

An alternative I posted in #1815, which would allow "on the fly" creation of contexts:

Right now we show a drop-down when multiple contexts are available:

selection_117

But what you'd need to be able to do is define a chain of relationships (or pull from an arbitrary static ID) to get to the user you wanted. So from a node you might need to follow the author. Something like this:

selection_119

But you wouldn't stop at that, you would need to be able to click the "+" to dig deeper, loading the user and then getting any relationships from that as well.

This idea here is basically that instead of defining relationships "up front", you'd do it as part of the process of adding a condition/block. I can see where you're coming from, but I'm not sure this would be friendly to more experience side-builders. You frequently want the same context over and over again. If you're positioning blocks from a user (referenced by a node) you wouldn't want to reselect the same chain of relationships every time you added a block or configured a condition. So we'd probably still want some way to save existing contexts that had already been added once.

So as quoted, we're probably going to need an "up-front" system of specifying contexts even if we also have an "on-the-fly" system. Should we start with that and work on an on-the-fly system later?

@klonos
Copy link
Member

klonos commented Sep 4, 2016

👍 @quicksketch thank you for filing this issue. I would say start with whichever way seems easier to implement. That will bring the feature faster to the users. Then we can compliment it by adding another (perhaps even easier) way to achieve the same thing. While we polish this, the users won't complain about a "missing" feature 😉

@docwilmot
Copy link
Contributor

An initial idea for starters

  • creates a Relationship Class which can be extended by Relationship plugins.
  • only a user_from_node plugin is included so far
  • all possible relationships are available to a layout; for example for a layout with path 'node/%', the Node author would be available automatically. There is no UI to restrict which relationships to add to a layout
  • if more than one context is available when adding a block, the existing UI allows you to choose. This now works with authors too: on 'node/%' layouts, any block which requires a 'user' context will now allow you to choose if the current user or the node author is to be passed to the block
  • a test block is added to the PR just for testing User required contexts
  • but if a block requires a 'Node' and a 'User' context, there is no UI to match node author to node yet.
  • doc blocks are inaccurate, many copy paste bloopers.

@quicksketch would appreciate your feedback please

@quicksketch
Copy link
Member Author

Woo! This is very exciting, thanks @docwilmot! This weekend is occupied with the 1.6.0 release, but I hope I'll be able to take a look at it next week.

@Al-Rozhkov
Copy link
Member

Al-Rozhkov commented Apr 14, 2017

I managed to crush sandbox :) I was trying to display field from node author account. And every node return 500 error.

My steps:

  1. Add image field to user account.
  2. Add new user with Administrator role.
  3. Login with new user and create node.
  4. Add field as block (image field from author account) on node/% layout.

Interesting thing. I can't even save layout with relationship context field when I'm logged as User2. It is return WSOD after clicking "Save layout". As User 1 I can save layout, but node pages return 500 error.

@quicksketch
Copy link
Member Author

Based on feedback from @Al-Rozhkov seems like this needs work. I still would like to review it when I get a chance even it needs work.

@jenlampton
Copy link
Member

It sounds like this issue is too alpha to make it into Backdrop 1.7, but I think we should make it a priority for Backdrop 1.8. (Bumping milestone)

@jenlampton jenlampton modified the milestones: 1.8.0, 1.7.0 Apr 30, 2017
@jenlampton
Copy link
Member

Thinking about this more, we might want to add the relationships at the time we're choosing or placing a block, instead of separately at the layout level. It's a bit strange from a UX perspective to add the relationship before you know you need it. I don't know how hard that would be to do based on what we already have, but if we aren't going to get this done for the 1.8 release it might be worth thinking about.

@jenlampton
Copy link
Member

Bumping to 1.9 as per our meeting today, @quicksketch will elaborate here shortly.

@jenlampton jenlampton modified the milestones: 1.9.0, 1.8.0 Aug 17, 2017
@jenlampton
Copy link
Member

No work on this since the last release. removing milestone. (Should we get a PR ready, one can always be added back!)

@jenlampton jenlampton removed this from the 1.9.0 milestone Jan 15, 2018
@docwilmot
Copy link
Contributor

FYI just listened to the meeting today. This actually needs committer review now, I havent got any further changes in mind at this time.

@herbdool
Copy link

@docwilmot I reviewed your notes and also added a couple comments. But I haven't done a full review. From what I understand, it seems that since the last review by @quicksketch you've fixed some tests. Does that mean it's pretty much already reviewed? I'll try find time to review more closely either way.

@quicksketch
Copy link
Member Author

I also added a code review to the PR that has a few suggested changes: backdrop/backdrop#3422 (review)

@quicksketch
Copy link
Member Author

I implemented my suggested changes that build on the current PR: docwilmot/backdrop#6

@docwilmot will need to review that PR to merge it into his branch. Or if needed I can make a new PR that incorporates it separately. I'd just love get @docwilmot's feedback before creating a new one.

@klonos
Copy link
Member

klonos commented May 1, 2021

@quicksketch I've asked a question in your PR re constants + a suggestion of what I think might be an oversight.

@quicksketch
Copy link
Member Author

Thanks @klonos! Answered the question and nice catch on the mistake I made there. I filed the complete changes in a new PR at backdrop/backdrop#3611 so that I can get the test suite running against it.

@herbdool
Copy link

herbdool commented May 2, 2021

I've realized that I don't understand the best way to test this again. I thought that it might expose fields like author, but I guess that's a pseudo field. Since it's specifically the author of the node I'm not sure if anything is exposed in a block. Currently is it just visibility condition that it provides more options?

@herbdool
Copy link

herbdool commented May 2, 2021

I've answered my own question. I added a field on the user account, then added it to the layout, with using the context of the author. Shows up correctly.

@quicksketch
Copy link
Member Author

I updated the PR at backdrop/backdrop#3611 with final changes suggested by @herbdool and all tests should now be passing again.

  • User from Node has been renamed to "Author from Content" everywhere in the front-end UI (internally it's still AuthorFromNode, which is fine/consistent with other references to nodes).
  • I simplified the API a bit so that Layout::getContexts() returns relationships by default, though they can be filtered out if needed by passing in a parameter.
  • Improved the "broken" relationship handler so that it displays correctly if a handler has been removed (such as when disabling a module).

@jenlampton
Copy link
Member

Code review looks great, my only feedback are suggestions for user-facing text, which shouldn't block the merge. Doing some manual testing now :)

@jenlampton
Copy link
Member

jenlampton commented May 3, 2021

This looks fantastic, only two small nits from the my manual testing review:

  1. There seem to be two UL tags with the class action-links being added to the page, rather than two links in the same list:

Screen Shot 2021-05-02 at 6 49 21 PM

  1. Two instances of "Node" are appearing in the UI (should be Content instead)

Screen Shot 2021-05-02 at 6 57 59 PM

@quicksketch
Copy link
Member Author

I merged backdrop/backdrop#3611 into 1.x for 1.19.0! 🎉

Thank you SO MUCH @docwilmot for taking on this complex need and making it happen!

Minor UI changes like combining the action links as suggested by @jenlampton can be done before the 1.19.0 release. We have a number of follow-ups that we could build on here, such as adding support for the other reference handlers that were removed, such as File from field and Taxonomy terms.

@herbdool
Copy link

herbdool commented May 3, 2021

@jenlampton should we have a follow-up issue for your two suggestions so we don't lose track of them?

@jenlampton
Copy link
Member

jenlampton commented May 3, 2021

I'm working on a PR for the action links now, hoping we can get it in tonight.

Working on it over in #5074

@herbdool
Copy link

herbdool commented May 3, 2021

Thanks for explaining bitwise operators in the PR comments @quicksketch. I see now why it made sense to use for the context usage types. It seemed wrong at first.

@jenlampton jenlampton changed the title Add support for "Relationships" to Layout Contexts Add support for "Relationships" to Layout Contexts. May 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment