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

Show the route configuration in the news feed page #6960

Merged
merged 3 commits into from Mar 8, 2024

Conversation

aschempp
Copy link
Member

@aschempp aschempp commented Mar 4, 2024

It sure makes sense to see that the RSS page has a .xml suffix, right? Not sure if the page can have parameters at all?

Bildschirmfoto 2024-03-04 um 15 31 33

@aschempp aschempp added the bug label Mar 4, 2024
@aschempp aschempp added this to the 5.3 milestone Mar 4, 2024
@aschempp aschempp requested a review from a team March 4, 2024 14:33
@aschempp aschempp self-assigned this Mar 4, 2024
@leofeyer
Copy link
Member

leofeyer commented Mar 7, 2024

There are two problems IMHO:

  1. The page cannot have parameters, so the {parameters} part is misleading.
  2. I now see potential route conflicts with other pages, although those are not .xml requests:

@fritzmg
Copy link
Contributor

fritzmg commented Mar 7, 2024

  1. The page cannot have parameters, so the {parameters} part is misleading.

This is something we can fix on the service tag of the page controller (I think?).

@leofeyer leofeyer changed the title Added route config to news feed page Show the route configuration in the news feed page Mar 8, 2024
@leofeyer leofeyer enabled auto-merge (squash) March 8, 2024 07:24
@leofeyer
Copy link
Member

leofeyer commented Mar 8, 2024

Thank you @aschempp.

@leofeyer leofeyer merged commit 6332f2d into contao:5.3 Mar 8, 2024
17 checks passed
@aschempp
Copy link
Member Author

aschempp commented Mar 8, 2024

why did you add the clr class? Did you check with all the other page types?

@aschempp aschempp deleted the fix/rss-path branch March 8, 2024 07:40
@leofeyer
Copy link
Member

leofeyer commented Mar 8, 2024

Yes, it was missing there, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants