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

Enhancement/issue 125 meta from config #303

Merged
merged 13 commits into from
Mar 23, 2020
Merged

Enhancement/issue 125 meta from config #303

merged 13 commits into from
Mar 23, 2020

Conversation

thescientist13
Copy link
Member

@thescientist13 thescientist13 commented Mar 16, 2020 •
edited
Loading

Related Issue

resolves #125 and builds off of / should be merged after #300

See the direct diff between this PR and #300

Summary of Changes

  1. Moves <meta> data to app-template.js, like with <title> and pull from GraphQL
  2. Remove instances of METAELEMENT, METADATA, METAIMPORT
  3. Add missing href from config schema
  4. Update documentation
  5. Update code coverage thresholds 💯

TODO (nice to have)

  1. Consider a meta specific qql query?

@thescientist13 thescientist13 added enhancement Improve something existing (e.g. no docs, new APIs, etc) CLI Content as Data labels Mar 16, 2020
hutchgrant
hutchgrant reviewed Mar 16, 2020
View reviewed changes
packages/cli/src/templates/app-template.js Outdated
this.setMeta(config.meta, currentPage);
}

setDocoumentTitle(title = '') {
Copy link
Member

@hutchgrant hutchgrant Mar 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

sp. setDocumentTitle()

hutchgrant
hutchgrant reviewed Mar 16, 2020
View reviewed changes
packages/cli/src/templates/app-template.js
titleElement.innerHTML = title;
}

setMeta(meta = [], currentPage = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

I really liked this in a separate component as people will want to do custom app-templates, it would be easier to simply import the metaComponent. Thoughts?

hutchgrant
hutchgrant reviewed Mar 16, 2020
View reviewed changes
packages/cli/test/cases/build.default.workspace-template-app/src/templates/app-template.js Outdated
this.setDocoumentTitle(config.title);
}

setDocoumentTitle(title) {
Copy link
Member

@hutchgrant hutchgrant Mar 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

sp. setDocumentTitle() or is this on purpose because its reserved or something?

hutchgrant
hutchgrant requested changes Mar 16, 2020
View reviewed changes
Copy link
Member

@hutchgrant hutchgrant left a comment

Choose a reason for hiding this comment

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

Spelling mistake on function, I don't know if that was intentional. Is it not easier for other developers looking to make their own app-templates if we can refactor the meta query and dom changes into a separate component ?

@thescientist13
Copy link
Member Author

thescientist13 commented Mar 18, 2020 •
edited
Loading

Spelling mistake on function, I don't know if that was intentional.

😬

Good catch. 👍

Is it not easier for other developers looking to make their own app-templates if we can refactor the meta query and dom changes into a separate component ?

I think that's definitely a good idea and so I think you are thinking of having something equivalent to react-helmet? I think that's a good idea so unless there's any strong objection, I think I would like to make an issue for that, primarily to determine if

  1. It should aliased within the CLI package like we do with data, e.g. @greenwood/cli/components/meta
  2. Or as its package, but as a dependency, e.g. @greenwood/component-meta

@thescientist13 thescientist13 self-assigned this Mar 18, 2020
@thescientist13 thescientist13 mentioned this pull request Mar 18, 2020
Closed
5 tasks
thescientist13
thescientist13 commented Mar 18, 2020
View reviewed changes
Copy link
Member Author

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Fixed the spelling mistake and opened an issue for tracking work around making a meta component in #304 .

Just need to make one last change to #300 and then this can be merged.

@thescientist13 thescientist13 removed their assignment Mar 18, 2020
@thescientist13
Rfc/issue 115 build time data access (#269)
c07a2a8 
* graphql server working

* apollo client connect to apollo server

* connected header example using lit apollo

* todo

* todos

* query and client + server refactor

* schema refactoring

* clean up console logging

* alias all @greenwood/cli/data module imports

* avoid paramater destructuring

* graphql example in the header

* multiple schemas

* internal data sources documentation

* shelf refactor and children query integration

* refactor out ApolloQuery

* ability to intercept client.query calls

* basic semi-working implementation

* remove extra config from server context

* have puppeteer wait for graphql requests before returning content

* fix and add test cases for apollo

* merged resolvers not actually working

* multiple queries support

* everything working

* todos

* TODO tracking

* fix fallback apollo client fetch handling

* full test suite

* cache json test cases

* stablize test due to inconsistent data results ordering

* clean up deps

* todo cleanup

* remove forced client call in SSG mode for client

* represent graph through the schema

* updated data docs

* typos and grammer

* typos and community link fixes
@thescientist13
hello and graph queries working together
e2e0645
@thescientist13
config def and resolvers
4b481aa
@thescientist13
flesh out config schema and query
0766c2c
@thescientist13
remove theme file from config
91899c2
@thescientist13
delete hello query example
b9f56f6
@thescientist13
formatting
3ce85f1
@thescientist13
documentation
428430f
@thescientist13
app template with config title from data
50bf397
@thescientist13
tests for custom page title
7220c11
@thescientist13
refactoring meta to app-template and data instead of scaffolding and …
e50e21d 
…added tests
@thescientist13
update coverage config
d4a339d
@thescientist13
fix spelling mistake
b5d98ee
@thescientist13 thescientist13 force-pushed the enhancement/issue-125-meta-from-config branch from 578031f to b5d98ee Compare March 22, 2020 23:54
thescientist13
thescientist13 commented Mar 22, 2020
View reviewed changes
Copy link
Member Author

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Made a follow up issue for creating a standalone <meta> component package. 👍

@thescientist13 thescientist13 merged commit cb8742c into release/0.5.0 Mar 23, 2020
@thescientist13 thescientist13 deleted the enhancement/issue-125-meta-from-config branch March 23, 2020 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hutchgrant hutchgrant hutchgrant requested changes

@aholtzman aholtzman Awaiting requested review from aholtzman

Assignees
No one assigned
Labels
CLI Content as Data enhancement Improve something existing (e.g. no docs, new APIs, etc)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thescientist13 @hutchgrant