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.

Sign up for GitHub

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

Jump to bottom

Add robots.txt setting. #5584

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add robots.txt setting. #5584

wants to merge 7 commits into from
+18 −40

Conversation

robgietema
Copy link
Member

@robgietema robgietema commented Dec 29, 2023 •
edited by ericof
Loading

Fixes #5580

@netlify Netlify
Copy link

netlify bot commented Dec 29, 2023 •
edited
Loading

✅ Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit ea4f692
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/67921ebca9ac3c000802bf82

@netlify Netlify
Copy link

netlify bot commented Dec 29, 2023 •
edited
Loading

✅ Deploy Preview for volto ready!

Name Link
🔨 Latest commit 442520b
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/6593d8ccc44d5b0008188cd6
😎 Deploy Preview https://deploy-preview-5584--volto.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@robgietema robgietema mentioned this pull request Dec 29, 2023
Open
4 tasks
stevepiercy
stevepiercy requested changes Dec 31, 2023
View reviewed changes
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Just a minor grammar fix to the change note, otherwise I approve the docs change only. A core developer should review this as well.

packages/volto/news/5580.feature Outdated Show resolved Hide resolved
sneridagh
sneridagh reviewed Jan 2, 2024
View reviewed changes
Copy link
Member

@sneridagh sneridagh left a comment

Choose a reason for hiding this comment

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

LGTM, but only make sure that the thing removed in the middleware is ok.

packages/volto/src/helpers/Robots/Robots.js Show resolved Hide resolved
packages/volto/src/helpers/Robots/Robots.js Show resolved Hide resolved
@sneridagh sneridagh mentioned this pull request Jan 2, 2024
Open
@sneridagh
Copy link
Member

@robgietema could you take a look at the suggestions and move this forward? Thanks!

@sneridagh
Merge branch 'main' into robots-txt-setting
ad3d9e1 
* main: (741 commits)
  Improve the usability of the ObjectBrowser when inputting a manual value, checking it on blur, and adding a local validator (#6576)
  fixing Markdown heading (#6588)
  fix site logo issue (#6591)
  Route registry (#6600)
  Release @plone/client 1.0.0-alpha.21
  Export the getContent bare fetcher (#6594)
  fix: incorrect copied state useClipboard (#6585)
  [RR7] Update to latest RR7 and conventions, fix index page (#6589)
  Release 18.6.0
  Release @plone/slate 18.1.0
  Revert "added swedish translation" (#6578)
  Slate Italian translations (#6563)
  Release 18.5.0
  Fix robots.txt in devmode (#6571)
  added swedish translation (#6557)
  Depth search issue (#6558)
  Block examples documentation (#6560)
  Fixed folder contents issues with persistent selection (#6554)
  Fix redirects to MDN responsive images (#6552)
  Bugfix remove query string inclusion in body class generation logic (#6547)
  ...
@sneridagh
Copy link
Member

@robgietema @ericof @davisagli Let's resurect this, I've merged it with main and solved conflicts.

@sneridagh sneridagh added this to the 18.x.x milestone Jan 21, 2025
ericof
ericof approved these changes Jan 21, 2025
View reviewed changes
Copy link
Member

@ericof ericof left a comment

Choose a reason for hiding this comment

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

LGTM

davisagli
davisagli requested changes Jan 22, 2025
View reviewed changes
Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

@sneridagh @ericof For the most part this is just updating the implementation details, but I see one breaking change that needs to be resolved, and the changelog says it does something it doesn't.

packages/volto/src/helpers/Robots/Robots.js Show resolved Hide resolved
packages/volto/news/5580.feature Outdated Show resolved Hide resolved
packages/volto/src/express-middleware/robotstxt.js Show resolved Hide resolved
@davisagli davisagli mentioned this pull request Jan 22, 2025
Merged
@sneridagh sneridagh requested a review from davisagli January 22, 2025 12:14
stevepiercy
stevepiercy requested changes Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Just a sprinkle of Markdown syntax.

I haven't been following the flurry of robots.txt and sitemap.xml things, so I'm not qualified to give a technical review, and I got my hands full with $WORK.

packages/volto/news/5580.feature Outdated Show resolved Hide resolved
@stevepiercy
Copy link
Collaborator

The readme-link-check failure should be temporary. I restarted it to verify.

docs/readthedocs.org will fail until I have a moment to fix it. Ignore for now.

@sneridagh
Copy link
Member

@robgietema @ericof @davisagli a final blessing, please.

davisagli
davisagli approved these changes Jan 23, 2025
View reviewed changes
Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

One new doubt, but I think we could merge this anyway.

packages/volto/src/config/ControlPanels.js
@@ -76,7 +76,6 @@ export const unwantedControlPanelsFields = {
'site_favicon_mimetype',
'exposeDCMetaTags',
'enable_sitemap',
'robots_txt',
Copy link
Member

Choose a reason for hiding this comment

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

@sneridagh Should we keep hiding it here if the VOLTO_ROBOTSTXT environment variable is set? Because then robots.txt is served based on the environment variable and not the setting.

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

@ericof ericof ericof approved these changes

@stevepiercy stevepiercy stevepiercy requested changes

@davisagli davisagli davisagli approved these changes

@sneridagh sneridagh Awaiting requested review from sneridagh

Assignees
No one assigned
Labels
None yet
Projects
Volto Team Meeting
Status: Work approved
Milestone
18.x.x
Development

Successfully merging this pull request may close these issues.

Fix robots.txt feature
5 participants
@robgietema @sneridagh @stevepiercy @davisagli @ericof