Navigation Menu

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 ambiguous UploadField labels #968

Merged
merged 2 commits into from Jul 22, 2019

Conversation

jonom
Copy link
Contributor

@jonom jonom commented Jul 18, 2019

‘Browse’ could mean your computer or the CMS File store. Likewise for ‘Add from files’, particularly since the word ‘Add’ implies a new file is being introduced to the system rather than a previous upload being reused. Fixes #765

@robbieaverill I did yarn install and yarn build in the root and got a ton of error messages. Is there a trick here? Or do builds happen somewhere else?

‘Browse’ could mean your computer or the CMS File store. Likewise for ‘Add from files’, particularly since the word ‘Add’ implies a new file is being introduced to the system rather than a previous upload being reused. Fixes silverstripe#765
@ScopeyNZ
Copy link
Member

As long as you yarn;yarn build from the root of asset-admin it should work. Ensure you're using the right version of node - nvm (node version manager) is your friend for this. For the 1 branch the version of node required is 10.

cc @silverstripeux

@jonom
Copy link
Contributor Author

jonom commented Jul 18, 2019

Thanks @ScopeyNZ - is this standard for all SS react projects? Is it documented somewhere and could we link to it from the contributing section of the ReadMe?

@ScopeyNZ ScopeyNZ closed this Jul 18, 2019
@ScopeyNZ ScopeyNZ reopened this Jul 18, 2019
@ScopeyNZ
Copy link
Member

Hit the wrong button sorry

This is standard for SS projects, and any npm/yarn project. We try to help out by specifying the node version required in package.json and .nvmrc. In saying that - our docs could always do with updating to help understanding for this 😅

@jonom
Copy link
Contributor Author

jonom commented Jul 18, 2019

Thanks @ScopeyNZ, I installed nvm and switched to v10, still bombed. Here's a sample:

gyp ERR! configure error
gyp ERR! stack Error: `gyp` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onCpExit ([redacted]/asset-admin/node_modules/node-sass/node_modules/node-gyp/lib/configure.js:336:16)
gyp ERR! stack     at ChildProcess.emit (events.js:198:13)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:248:12)
gyp ERR! System Darwin 18.6.0
gyp ERR! command "[redacted]/.nvm/versions/node/v10.16.0/bin/node" "[redacted]asset-admin/node_modules/node-sass/node_modules/node-gyp/bin/node-gyp.js" "rebuild" "--verbose" "--libsass_ext=" "--libsass_cflags=" "--libsass_ldflags=" "--libsass_library="
gyp ERR! cwd [redacted]asset-admin/node_modules/node-sass
gyp ERR! node -v v10.16.0
gyp ERR! node-gyp -v v3.6.2

Any chance you could do the build, if you're happy with this change?

@ScopeyNZ
Copy link
Member

Yep sure. We can do the build for you. I'll wait for some feedback from the UX team and @robbieaverill for the translation tag usage - he might not want the tags changed so that we don't lose translation coverage.

@jonom
Copy link
Contributor Author

jonom commented Jul 18, 2019

he might not want the tags changed so that we don't lose translation coverage.

Sure. I was inclined to use the existing tags for that same reason but @robbieaverill and @maxime-rainville thought it was probably safer to use new ones.

@jonom
Copy link
Contributor Author

jonom commented Jul 18, 2019

We can do the build for you

Thanks!

@maxime-rainville
Copy link
Contributor

I've rebuilt the client lib which should make the travis green.

@robbieaverill
Copy link
Contributor

robbieaverill commented Jul 22, 2019

Behat failure is pre-existing 😢

@robbieaverill robbieaverill merged commit 9681ba3 into silverstripe:1 Jul 22, 2019
@jonom jonom deleted the uploadfield-labels branch July 22, 2019 16:53
@jonom
Copy link
Contributor Author

jonom commented Jul 22, 2019

Thanks for the build and merge 👍

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.

Reword "Browse or Add from files"
4 participants