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

[#111]: Backport changes from ecosystem #161

Merged
merged 8 commits into from
Sep 14, 2022

Conversation

khalilcodes
Copy link
Contributor

@khalilcodes khalilcodes commented Sep 12, 2022

Backport changes to flowershow after ecosystem upgrade

Tasks from #111


This PR adds changes in User config (for version), Navbar title, Dark/Light theme switcher, Search component and flowershow image assets.

Tasks

  • Create _flowershow folder in public with images (logo and theme icon)
  • Add navbarTitle object in siteConfig.js specific to flowershow stuff (title text, version, logo, etc) revert changes
  • Create Navbar title function in components/Nav,js to render site title based off config values
    • values are from user's config.js and properties can be set in optional navbarTitle for text [String], logo [Boolean] and version [Boolean]
    • handle SEO title for both cases in pages/_app.js revert to previous config
  • Add conditional rendering for Search component if docsearch configs exist (ie., appId, apiKey and indexName)
    • Add a env.development file with test values to render search component in development mode
  • Add forced light theme and handle rendering of theme button based on config ie., if theme.default is an empty string
  • Amend name of url path field to url_path in. contentlayer.config.js to avoid conflicts if any

Notes

  • Searchbar component may not render in preview deploy as env variables will not be loaded. The searchbar rendering relies on having all three required algolia configs to be defined (ie. appId, apiKey and indexName) This works
  • upgrade.sh package needs to be updated to remove .env.development file

@netlify
Copy link

netlify bot commented Sep 12, 2022

Deploy Preview for spectacular-dragon-c1015c ready!

Name Link
🔨 Latest commit 0192243
🔍 Latest deploy log https://app.netlify.com/sites/spectacular-dragon-c1015c/deploys/6320c12c5ce4db000a27e045
😎 Deploy Preview https://deploy-preview-161--spectacular-dragon-c1015c.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.

@khalilcodes khalilcodes mentioned this pull request Sep 13, 2022
44 tasks
@khalilcodes khalilcodes requested a review from olayway September 13, 2022 17:53
Copy link
Member

@rufuspollock rufuspollock left a comment

Choose a reason for hiding this comment

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

I think code is ok for now though i have a few comments (don't need to change for them). Big thing missing is a bit of docs. I can do this quickly.

@@ -1,5 +1,5 @@
const config = {
title: 'Flowershow',
navbarTitle: { version: true },
Copy link
Member

Choose a reason for hiding this comment

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

surely navbarTitle is something like:

{
	title: 'Flowershow',
	logo: ...
	version: 'Alpha'
}

@@ -14,6 +14,36 @@ function GitHubIcon(props) {
);
}

function NavbarTitle() {
// include option to define different text from title (if needed) in config
const title = siteConfig.navbarTitle?.text || siteConfig.title
Copy link
Member

Choose a reason for hiding this comment

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

Personally, i would be strict and say if you use navbarTitle you have to set title in their too i.e. this is:

if (navbarTitle) {
	use stuff from navbarTitle
} else {
      use siteConfig.title
}

Why? Explicitness is better than magic!

@rufuspollock rufuspollock merged commit bfef4f6 into main Sep 14, 2022
@rufuspollock rufuspollock deleted the 111-flowershow-backport-changes branch September 14, 2022 07:15
rufuspollock added a commit to rufuspollock/flowershow that referenced this pull request Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants