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

Get Search working on PR instance #1050

Merged
merged 1 commit into from
Jul 8, 2022
Merged

Get Search working on PR instance #1050

merged 1 commit into from
Jul 8, 2022

Conversation

cjyabraham
Copy link
Contributor

@cjyabraham cjyabraham commented Jul 6, 2022

Currently the search form on PR instance redirects to the live Glossary site. This PR is to get it so that the form keeps the user on current instance.

If you go to this dev instance and search for something, you'll see the search results now appear on the same dev instance rather than going to the live site.

Signed-off-by: cjyabraham cjyabraham@gmail.com

Signed-off-by: cjyabraham <cjyabraham@gmail.com>
@netlify
Copy link

netlify bot commented Jul 6, 2022

Deploy Preview for cncfglossary ready!

Name Link
🔨 Latest commit bc7147e
🔍 Latest deploy log https://app.netlify.com/sites/cncfglossary/deploys/62c539d68e1d950008ed9535
😎 Deploy Preview https://deploy-preview-1050--cncfglossary.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 settings.

@cjyabraham cjyabraham marked this pull request as ready for review July 6, 2022 07:44
@jihoon-seo jihoon-seo added the maintainers Use this label if PR requires maintainers to take action label Jul 6, 2022
@CathPag
Copy link
Collaborator

CathPag commented Jul 6, 2022

Hmm...I just tested it and it still throws me back to the live site...or did I misunderstand what this PR is about?

@cjyabraham
Copy link
Contributor Author

You tested this instance? https://deploy-preview-1050--cncfglossary.netlify.app/

@CathPag
Copy link
Collaborator

CathPag commented Jul 6, 2022

Yep, just send you a screenrecording via Slack

Copy link
Collaborator

@CathPag CathPag left a comment

Choose a reason for hiding this comment

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

LGTM

@jihoon-seo
Copy link
Collaborator

@cjyabraham would there be no side effect from replacing baseURL in config.toml from "https://glossary.cncf.io/" to "/"? 😊

@cjyabraham
Copy link
Contributor Author

I don't think so as that is the way we do it over on the Contribute site: https://github.com/cncf/tag-contributor-strategy/blob/main/website/config.toml#L1

We should definitely test it thoroughly when we deploy.

Copy link
Collaborator

@jihoon-seo jihoon-seo left a comment

Choose a reason for hiding this comment

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

LGTM!

@iamNoah1 iamNoah1 merged commit 1d6e12a into main Jul 8, 2022
@cjyabraham cjyabraham deleted the search branch July 8, 2022 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainers Use this label if PR requires maintainers to take action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants