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

Fix custom nav links #4387

Merged
merged 7 commits into from
Feb 25, 2018
Merged

Fix custom nav links #4387

merged 7 commits into from
Feb 25, 2018

Conversation

frapan
Copy link

@frapan frapan commented Jul 19, 2017

Description of changes

There are 3 bugs that get fixed with this pull request:

  1. There was an error in the unit test about the accepted languages which made "npm test" fail (see file /test/unit/lib/middleware/language.js)
  2. In Keystone v0.3 it was possibile to add an object (or an array of objects) to the nav to make external links. In v4.0 it was only possibile to add an array of objects, forcing you to wrap the object inside an array. With this pull request you can add single objects again.
  3. If you add an object (inside an array) with an external link to the nav, v4.0 shows the link but it is erroneously wrapped inside a Link component and therefore the router cannot handle it. This behaviour has been changed so that external links are represented simply by "a" tags, not by Link components.

Related issues

The following issues are related to this pull request:

Testing

  • Please confirm npm run test-all ran successfully.

@frapan
Copy link
Author

frapan commented Jul 20, 2017

How do I know what's wrong with deploy/netlify? The deploy preview failed but I don't think it's because of my changes.
If I click on the Details link, I get a 404 (I signed up to netlify with github account).
Can anyone help me?

@frapan
Copy link
Author

frapan commented Jul 30, 2017

I've just realized that the netlify deploy went wrong beacuse of the commit about the nav section as simple objects.
Should I assume that this behaviour is not wanted anymore?
Should I remove that commit?
Or the netlify deploy should be fixed to let nav sections be simple objects?

@frapan
Copy link
Author

frapan commented Jul 30, 2017

@JedWatson can you please answer me? Or could you please invite me to the slack channel in order to discuss about theese questions?

@frapan
Copy link
Author

frapan commented Jul 31, 2017

@mxstbr can you help me with this pull request? I have no access to the netlify logs (see my comments above) and I don't know what the problem is.
I tried to reset the initNav.js as it was before my pull request but it keeps on giving me the same netlify failure.
How can I work it out? If you invite me to the Keystone slack, do you think I could find an answer?
Thanks.

@@ -69,7 +69,7 @@ describe('language', function () {
});
var expected = 'zh-CN';
var req = mockRequest({
acceptLanguage: 'zh-CN;1,en-US;q=0.8'
acceptLanguage: 'zh-CN;q=1,en-US;q=0.8'
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @frapan, sorry for the delay in getting back to you. The only thing I don't understand in your PR is the change to this file. Could you let me know what this is doing for us? I'd be happy to merge the other changes, just want to make sure I understand what's happening here.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you @blargity for your reply and I'm sorry as well for being late.
I had an error during tests because of that line, and I couldn't go on with the tests.
In fact, the syntax of acceptLanguage expects the weights of the languages to be written with ";q=", not simply ";".
See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Language as a reference.

@Noviny
Copy link
Contributor

Noviny commented Sep 22, 2017

@frapan Thankyou for some more patience!

I was wondering if you would mind documenting this feature, likely on this page. I like my hidden features in video games, but less so in my node modules. 😛

@frapan
Copy link
Author

frapan commented Oct 24, 2017

@Noviny I agree with you. I've just added the documentation to the page you suggested.

@frapan frapan closed this Oct 24, 2017
@frapan frapan reopened this Oct 24, 2017
@Noviny Noviny merged commit 3bdbe5f into keystonejs:master Feb 25, 2018
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.

3 participants