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

Remove unnecessary 'Delete' title from 'Revoke' menu entry #12186

Closed
wants to merge 1 commit into from

Conversation

jancborchardt
Copy link
Member

This looked off:
delete tooltip for revoking devices

So I removed the extra tooltip. Please review @nextcloud/designers :)

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt jancborchardt added bug design Design, UI, UX, etc. 3. to review Waiting for reviews feature: settings labels Nov 1, 2018
@jancborchardt jancborchardt added this to the Nextcloud 15 milestone Nov 1, 2018
@juliusknorr
Copy link
Member

@jancborchardt For changed handlebars templates you need to run ./build/compile-handlebars-templates.sh. No need to commit the js/ changes here, I even wonder why there are changes.

@juliusknorr
Copy link
Member

Btw. maybe we should think about adding proper makefiles to the server, so it is easier to just run what needs to be run for settings or apps. cc @skjnldsv @ChristophWurst

@skjnldsv
Copy link
Member

skjnldsv commented Nov 1, 2018

Btw. maybe we should think about adding proper makefiles to the server, so it is easier to just run what needs to be run for settings or apps. cc @skjnldsv @ChristophWurst

Since we want to remove the handlebars, I'd say this is a temp solution and we should remove this for 16

@rullzer
Copy link
Member

rullzer commented Nov 1, 2018

Btw. maybe we should think about adding proper makefiles to the server, so it is easier to just run what needs to be run for settings or apps. cc @skjnldsv @ChristophWurst

Feel free to submit something. Even if we eventually want to get rid of handlebars. Still a simple make file to do a lot of heavy lifting would already help :D

@jancborchardt
Copy link
Member Author

Btw. maybe we should think about adding proper makefiles to the server, so it is easier to just run what needs to be run for settings or apps.

Yes please. I am fairly involved in Nextcloud (right? :D) and even I have no idea where to do which make and dev-setup and npm install and handlebars stuff. And where / if to commit compiled files. It’s confusing.

@MorrisJobke
Copy link
Member

But the diff looks weird - "+63,025 −1,404" 🙈

@jancborchardt
Copy link
Member Author

Oh and about the handlebars build step, I’m getting errors I could not resolve through searching yesterday:
Error: Cannot find module 'optimist'

Any idea? How did you install handlebars? (I needed to do via apt get cause otherwise the handlebars command was not found.) Need some docs :)

I will create a separate pull request then cause this is messed up.

@MorrisJobke
Copy link
Member

Any idea? How did you install handlebars? (I needed to do via apt get cause otherwise the handlebars command was not found.) Need some docs :)

I will create a separate pull request then cause this is messed up.

I guess we should do this in the install command via npm/yarn. Installing from the repos will give you super old stuff. If it is not the the path, then you either add the node_modules/bin to the path or always call it with the full path ;)

@danxuliu
Copy link
Member

danxuliu commented Nov 1, 2018

Any idea? How did you install handlebars?

If it is of any help, I run the Handlebars compiler inside the same Docker image used by Drone with:

docker run --rm --volume /PATH/TO/YOUR/NEXTCLOUD/DEVELOPMENT/DIRECTORY:/nextcloud --name nextcloud-handlebars --interactive --tty node bash

This opens a Bash session inside the container (which will be automatically removed once the Bash session ends). Then, in that Bash session I run:

cd /nextcloud
npm install handlebars -g

And then compile the Handlebars templates by running (also in the Bash session inside the container):

./build/compile-handlebars-templates.sh

This way I keep my system clean from NPM :-P

@jancborchardt
Copy link
Member Author

Thanks! So possible short docs:


We still use Handlebars templates some places in Files and Settings. We will replace these step-by-step with Vue.js, but in the meantime you need to compile them separately.

If you don’t have Handlebars installed yet, you can do it with this terminal command:

sudo npm install -g handlebars

Then inside the root folder of your local Nextcloud development installation, run this command in the terminal every time you changed a .handlebars file to compile it:

./build/compile-handlebars-templates.sh

When making changes, also commit the compiled .js files!


Ideally we should have all that quickstart-stuff in the README.md, right now it’s only a link to the docs. I would like to add this, and a section on the Vue compilation with make dev-setup etc.

I will move the contents of https://github.com/nextcloud/server/blob/master/settings/README.md (cause no one finds that) into the main readme to a section called 👩‍💻 Development setup, and add my part about Handlebars. Anyone opposed to that?

@jancborchardt
Copy link
Member Author

Hehe @danxuliu we duplicated text ;D Sorry

@jancborchardt
Copy link
Member Author

Replacement pull request at #12191 – need some small JS help with triggering the action on pressing enter. :)

Will do a pull request for the documentation to readme too.

@jancborchardt
Copy link
Member Author

As mentioned, pull request for documentation in readme is open at #12192 – please review. :)

@MorrisJobke MorrisJobke removed this from the Nextcloud 15 milestone Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug design Design, UI, UX, etc. feature: settings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants