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

ContentNode: Create new responsive layout #4071

Merged
merged 42 commits into from Dec 17, 2023

Conversation

manuelmeister
Copy link
Member

@manuelmeister manuelmeister commented Nov 10, 2023

This PR introduces the new ResponsiveLayout, building upon the prototype showcased in POC #3966. The implementation should make it easier to tailor the layout for different media and includes support for both print variants.

If you want to see the responsive layout in action: https://pr4071.ecamp3.ch/camps/3c79b99ab424/GRGR/program/activities/12f34c89ce11

Key Features

  • Implement new ContentNode called ResponsiveLayout
  • Adjust all dev data to new ResponsiveLayout
  • Implement ResponsiveLayout for both print variants
  • Prevent ResponsiveLayout encapsulation
  • Create additional tests for ResponsiveLayout and move generic to RootColumnLayout tests

Closes #3966
Closes #3628

@manuelmeister manuelmeister added type: Frontend UX/UI deploy! Creates a feature branch deployment for this PR External feedback This Issue is Based on Feedback from the Community labels Nov 10, 2023
Copy link

github-actions bot commented Nov 10, 2023

Feature branch deployment currently inactive.

If the PR is still open, you can add the deploy! label to this PR to trigger a feature branch deployment.

@carlobeltrame
Copy link
Member

Thanks, this looks to be coming along nicely. I'll state again in written form here what I have previously expressed orally:

I get the feeling you don't want to adjust all the camp prototypes and dev data, and adjust both prints, because it's a lot of work and you aren't sure yet the rest of the core team will approve. This is a clear sign for me that this shouldn't (yet) be called default layout. Put another way: If this new feature were clearly the new commonly agreed-upon default, there would be no doubts about changing the camp prototypes, dev data and print logic.

So in the state of this PR right now, I would prefer to call it flexible layout for now, and then maybe if it really turns out to be absurdly useful except in a few edge cases, rename it in the future to default layout or whatever seems fitting then. If we merge a new feature called default in an only partially complete form, we cannot do any releases until this "default feature" has been completed and thoroughly tested.

@manuelmeister
Copy link
Member Author

@carlobeltrame I just saw your comment after I updated the whole dev data (except lorem ipsum camp). 😄

Maybe you misunderstood me. I was (and still am) eager to provide the implementations and will continue this PR until it is ready for review. I've now added an implementation for nuxt that works and working on the vue3 pdf print one.

I think it makes sense to introduce this PR after #4003.

@manuelmeister
Copy link
Member Author

Can someone help me with the flakey query count snapshot test? I don't understand why this behaves so erratic.
Error: https://github.com/ecamp/ecamp3/actions/runs/7071319324/job/19248921634#step:10:66

Try to fix with commit: 9908ae9#diff-e8620ddac3c501cd95265fa71310036a686d6a1376608ed804d001937c88149cR13

New error: https://github.com/ecamp/ecamp3/actions/runs/7075762851/job/19258251668#step:10:66

@manuelmeister
Copy link
Member Author

@BacLuc I checked GRGR and it seems there were some missing. Now GRGR & Harry Potter should correctly use the new layout.

@BacLuc BacLuc removed the deploy! Creates a feature branch deployment for this PR label Dec 4, 2023
@manuelmeister manuelmeister added the deploy! Creates a feature branch deployment for this PR label Dec 4, 2023
@manuelmeister manuelmeister changed the title Create new responsive layout ContentNode: Create new responsive layout Dec 8, 2023
…ntent_node endpoints

With the ResponsiveLayout, we now have mostly ContentNodes which are nested by 2 levels:
ColumnLayout -> DefaultLayout -> "Content Nodes with content"
This may lead to n+1 db queries that we experienced in the EndpointQueryCountTest.
This change now allows to define a range for the query count.
This is good enough to keep the performance at the same level.
@BacLuc
Copy link
Contributor

BacLuc commented Dec 10, 2023

@ecamp/core
I have 2 proposals to fix the query count test:

  1. ContentNodeRepository: also fetch root and parent of contentNode manuelmeister/ecamp3#4
  2. EndpointQueryCountTest: allow a range for the number of queries on co… manuelmeister/ecamp3#5

If someone has an opinion on how to move forward with the query count test.

the third more invasive solution would be:
3. An Activity/Category has either a ColumnLayout or a ResponsiveLayout as root ContentNode. (i did not try if this even would work)

@manuelmeister
Copy link
Member Author

manuelmeister commented Dec 12, 2023

Context:

With this PR the query count of /content_nodes & /content_node/<entity> is flaky. Sometimes the query count is N+1. This issue probably occurs, because Doctrine wants to ensure the inheritance strategy:

This is the additional query it creates
SELECT
  t0.id AS id_1,
  t0.createTime AS createtime_2,
  t0.updateTime AS updatetime_3,
  t0.data AS data_4,
  t0.slot AS slot_5,
  t0.position AS position_6,
  t0.instanceName AS instancename_7,
  t0.rootId AS rootid_8,
  t0.parentId AS parentid_9,
  t0.contentTypeId AS contenttypeid_10,
  t0.strategy
FROM
  content_node t0
WHERE
  t0.id = ?
  AND t0.strategy IN (
    'contentnode', 'multiselect', 'materialnode',
    'storyboard', 'singletext', 'responsivelayout',
    'columnlayout'
  )

Solutions

ContentNodeRepository: also fetch root and parent of contentNode

Is this solution future proof, or does it only hide the problem for our tests? Because there we left join the parent of the parent, but the ColumnLayout (and future layout nodes) could be infinitely nested.

As I planned to create a FlexLayout, that lets you have the layout behavior as aside-top and aside-bottom (i.e. just a flex row with flex wrap) and the FlexLayout could be used/nested the same as ColumnLayout, this would probably make a performant/reliable solution necessary.

Maybe it would make sense to create a LayoutContentNode base class? And figure out the problem of layout nodes in a separate PR more generally? There we can find out what we need to accommodate future layouts.

EndpointQueryCountTest: allow a range for the number of queries

Allow this flaky behavior and allow query count to be a range.

Decision

I would vote for #5 adjust testing 🚀 and then let us investigate #4 modify QueryBuilder 👀 in a separate PR.

@simfeld
Copy link
Contributor

simfeld commented Dec 13, 2023

I would vote for manuelmeister#5 and then let us investigate https://github.com/manuelmeister/ecamp3/pull/4in a separate PR.

@manuelmeister your proposal sounds good to me

EndpointQueryCountTest: allow a range for the number of queries on co…
Copy link
Member

@pmattmann pmattmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have my doubts whether users understand how best to use this layout variant, but a field test can probably provide a better answer than my opinion.

Nevertheless, I would like to share my thoughts:
The ResponsiveLayout is a bigger step away from WYSIWYG than anything before. For me, the following example illustrates this particularly well:
If you insert ContentNodes into a side slot, their rendered size depends on how many ContentNodes are in that slot and on its position within the side slot.
I am curious to see whether our users will be able to cope with this.

Example:
Full width:
image

Paper width:
image

@BacLuc
Copy link
Contributor

BacLuc commented Dec 17, 2023

You need to merge devel again after #4136

@manuelmeister
Copy link
Member Author

I think it wont be such a big step, as papersize will be the default and then you intentionally toggle to wide size. So you can observe the changes live in the viewport. Maybe we can add the info that a multiple of 2 looks better.

@manuelmeister manuelmeister added this pull request to the merge queue Dec 17, 2023
Merged via the queue into ecamp:devel with commit ffc91bf Dec 17, 2023
28 checks passed
@manuelmeister manuelmeister deleted the feature/new-flexlayout branch December 17, 2023 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy! Creates a feature branch deployment for this PR External feedback This Issue is Based on Feedback from the Community type: Frontend UX/UI
Projects
Development

Successfully merging this pull request may close these issues.

Provide a content layout that is optimized for each media
5 participants