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

Make front-matter label var optional #58

Merged
merged 15 commits into from
Apr 28, 2019

Conversation

hutchgrant
Copy link
Member

@hutchgrant hutchgrant commented Apr 27, 2019

Related Issue

Resolves #30

Summary of Changes

Because of the bug in #54, the root route doesn't appear to be rendering correctly. I suggest you run yarn install && yarn serve then check http://localhost:8000/hello to see it works and then take a look at source html to see it's using a generated element label with the page-template.

This will fail the test because it's looking for wc-md-hello which doesn't exist with generated labels like: wc-md-rjrwbq for example. This is because I just removed it from the var from template for now, we can add it back once you've tried it out.

Copy link
Member

@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.

Nice 🎉

Should we get a test case for this? Basically make sure that both defaults work, but also providing overrides would work? (or doesn't? not sure what would be expected in these cases, but tests would help make it clear 👌 )

const letters = 'abcedfghijklmnopqrstuvwxyz';
let short = [], rand = 0;

for (let n = 0; n < size; n = n + 1) {
Copy link
Member

@thescientist13 thescientist13 Apr 27, 2019

Choose a reason for hiding this comment

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

a forEach or map might be nice here

Copy link
Member Author

@hutchgrant hutchgrant Apr 27, 2019

Choose a reason for hiding this comment

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

But then it has to pick random letters in order. That isn't random. This a popular one from stackoverflow that I've used for multiple projects(albeit usually with multiple cases and numbers).

Copy link
Member

Choose a reason for hiding this comment

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

nm, size is an int 😊

@thescientist13 thescientist13 added the enhancement Improve something existing (e.g. no docs, new APIs, etc) label Apr 27, 2019
@hutchgrant
Copy link
Member Author

hutchgrant commented Apr 28, 2019

New Changes

  • README updated

  • Added a unique test app specifically for testing custom front matter overrides.

  • Reverted master tests of default template that I broke for my initial demonstration.

  • Additional tests that will check for default randomly generated label and that user set label variable works.

  • Additional tests that will check working with custom and default templates.

  • Because of Enabling default app/page templates for user workspace #52 and Fix Multi Env Templates #45 I had to include a lot of unnecessary and duplicate files within the fixtures/custom-fm app. e.g. index.html and 404 templates, as well as app-template.js , and page-template.js They're all made redundant by those other PRs so I'm not going to write a workaround, I'd rather just remove them when the time comes.

Don't let the number of files deceive you, most are part of the new fixture for seven different tests and 2 files were just being reverted back to their original state.

Again, with all these PRs merged, it would be a lot easier to sort out the tests.

@hutchgrant
Copy link
Member Author

hutchgrant commented Apr 28, 2019

A thought, maybe we shouldn't revert the default template markdown. Instead, we should update their tests so that the front-matter var is no longer used by default(without default template markdown pages).

@hutchgrant
Copy link
Member Author

hutchgrant commented Apr 28, 2019

I set all the mock-app pages to no longer use the front-matter label var. So by default, no front-matter label var is being tested from custom user workspace(though it is still being used in default templates folder). I added a page which will use a custom template and use a label. So we're now testing both front-matter var from a single page(no need for seperate fixture).

Copy link
Member

@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.

👏 🏆

const letters = 'abcedfghijklmnopqrstuvwxyz';
let short = [], rand = 0;

for (let n = 0; n < size; n = n + 1) {
Copy link
Member

Choose a reason for hiding this comment

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

nm, size is an int 😊

README.md Outdated
# Helloworld
```
````
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be only be 3 ticks (`) in markdown?

Copy link
Member Author

@hutchgrant hutchgrant Apr 28, 2019

Choose a reason for hiding this comment

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

yes, I was confused because we used 4 when we had to show a render block within the markdown as an example.

e.g.

```render
<style>${CSS}</style>
<my-component></my-component>
```

]
loader: 'wc-markdown-loader',
options: {
graph
Copy link
Member

Choose a reason for hiding this comment

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

It's like magic ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve something existing (e.g. no docs, new APIs, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

As a user I would like default page template values based on filepath
2 participants