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 error building FieldTypes on Linux #3793

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix error building FieldTypes on Linux #3793

wants to merge 1 commit into from

Conversation

AurelioDeRosa
Copy link
Contributor

Description of changes

Set the first letter of the field to uppercase

Related issues (if any)

Fixes #3754

Testing

  • Please confirm npm run test-all ran successfully.

@jstockwin
Copy link
Contributor

Would it be worth instead using the keyToLabel function from keystone-utils here? Or are the two operations not quite the same? Seems to me that we should use the utils functions where possible, as they should be pretty good at handling all possible cases correctly.

https://github.com/keystonejs/keystone-utils/blob/master/lib/index.js#L636

@AurelioDeRosa
Copy link
Contributor Author

As far as I can tell, only the first letter needs to be uppercased, but you surely know the app better than I do.

@jstockwin
Copy link
Contributor

Haha, don't be too sure of that. You're probably right.

@jstockwin
Copy link
Contributor

We have to end up with camel-cased file names right, e.g. "CloudinaryImage", so I guess it depends what the input is. I'm sure your way works fine, but I think using the utils function should too.

I guess it doesn't really matter that much anyway though, so maybe just leave it.

@AurelioDeRosa
Copy link
Contributor Author

Cool then :)

@bassjacob bassjacob self-assigned this Dec 22, 2016
@JedWatson
Copy link
Member

We should actually be using the properName property on the node implementation for the Field Type. cc/ @bassjacob

@AurelioDeRosa
Copy link
Contributor Author

Let me know how to update the PR to have it landed.

@jstockwin
Copy link
Contributor

Does fieldTypes[type].properName work?

@iamstiil
Copy link
Contributor

iamstiil commented Feb 22, 2017

I don't think fieldTypes[type].properName would work.

My fieldTypes looks like this:

{
  boolean: 'Boolean',
  number: 'Number',
  text: 'Text',
  select: 'Select',
  file: 'File',
  color: 'Color',
  grid: 'Grid',
  datetime: 'Datetime',
  relationship: 'Relationship',
  textarea: 'Textarea',
  name: 'Name',
  email: 'Email',
  password: 'Password'
}

Grid is a Custom-Fieldtype, so it can be ignored.

Edit:
Seems like _.upperFirst does not fix the issue.

My errors look like Cannot find module 'types/password/PasswordFilter' from '/var/www/server/node_modules/keystone/admin/client'.

Keystone tries loading from '/var/www/server/node_modules/keystone/admin/client' instead from '/var/www/server/node_modules/keystone/fields'

Edit 2:
My Repo was not up to date. It fixed itself ^^

@bassjacob bassjacob removed their assignment Jul 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants