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

Moved name config option from Keystone to Admin UI #3227

Merged
merged 6 commits into from Jul 5, 2020

Conversation

Vultraz
Copy link
Contributor

@Vultraz Vultraz commented Jul 3, 2020

As says, since it's no longer used by the KS core and only passed to the Admin UI.

@changeset-bot
Copy link

changeset-bot bot commented Jul 3, 2020

🦋 Changeset is good to go

Latest commit: ab42016

We got this.

This PR includes changesets to release 20 packages
Name Type
@keystonejs/app-admin-ui Minor
@keystonejs/demo-project-blog Patch
@keystonejs/demo-custom-fields Patch
@keystonejs/demo-project-meetup Patch
@keystonejs/demo-project-todo Patch
create-keystone-app Patch
@keystonejs/keystone Major
@keystonejs/test-utils Patch
@keystonejs/cypress-project-access-control Patch
@keystonejs/cypress-project-basic Patch
@keystonejs/cypress-project-client-validation Patch
@keystonejs/cypress-project-login Patch
@keystonejs/cypress-project-social-login Patch
@keystonejs/adapter-knex Patch
@keystonejs/adapter-mongoose Patch
@keystonejs/benchmarks Patch
@keystonejs/example-projects-blank Patch
@keystonejs/example-projects-nuxt Patch
@keystonejs/example-projects-starter Patch
@keystonejs/example-projects-todo Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@timleslie
Copy link
Contributor

Haha yep, this is exactly where I was going with the changes down in the adapter code.

I'm just pondering the timing of this change, and whether we want to bundle it with this big batch of major changes, or wait for one more cycle. Let me have a think about it. I'm 💯 in favour of the change in principle 👍

@Vultraz
Copy link
Contributor Author

Vultraz commented Jul 3, 2020

I think it makes sense to include with the others. It logically goes with the change to db names.

@@ -11,6 +11,7 @@ const getWebpackConfig = require('./server/getWebpackConfig');

class AdminUIApp {
constructor({
name,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we default this to: name = 'Keystone'?

Copy link
Contributor

@timleslie timleslie left a comment

Choose a reason for hiding this comment

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

OK, I'm board with this.

There are a few instances of the old pattern used within the docs. initial-data.md and new-project.md for example. Could I get you to do a search through all the docs to make sure they're all updated as well 👍

@Vultraz
Copy link
Contributor Author

Vultraz commented Jul 5, 2020

@timleslie done

@timleslie
Copy link
Contributor

OK, I'm board with this.

Oops, on board, definitely not bored! 😅

Copy link
Contributor

@timleslie timleslie left a comment

Choose a reason for hiding this comment

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

🎉

@timleslie timleslie merged commit 49984ca into keystonejs:master Jul 5, 2020
@github-actions github-actions bot mentioned this pull request Jul 6, 2020
@Vultraz Vultraz deleted the name-config-to-admin-ui branch July 6, 2020 00:20
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.

None yet

2 participants