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

Use a nicer documents chooser help message if no documents exist #4859

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@nott
Contributor

nott commented Oct 27, 2018

@m1kola reported in #3838 that the document chooser shows the following message by default when the side doesn't have any documents:

Sorry, no documents match ""

image

@gmmoraes tried to fix this issue in #3845. He got some feedback in the PR: they suggested to add "add one document" button.

This PR implements those ideas, add tests and a JS helper function that switches to "UPLOAD" tab if available:

image

nott added some commits Oct 27, 2018

Use a nicer documents chooser help message if no documents exist
This improves helper text at documents chooser. If no documents exist
in DB, we used to display `Sorry, no documents match ""`. Let's make
it clear that no documents exist and suggest to add one if the user
has permissions to do that.

@nott nott force-pushed the nott:documents-chooser-helper-text branch from 6543147 to 276f2c7 Oct 27, 2018

@zerolab

Nice touch with the collection and upload form variations, @nott
Tests read well and are clear. 👍 on the login method changes -- will result in more DRY in tests.

Needs a quick front-end review, but LGTM!

@@ -26,13 +26,14 @@ def create_test_user():
return user_model.objects.create_superuser(**user_data)
def login(self):
user = self.create_test_user()
def login(self, user=None):

This comment has been minimized.

@zerolab

zerolab Oct 28, 2018

Contributor

👌

$('a.upload-one-now').on('click', function(e) {
// Set current collection ID at upload form tab
let collection_id = $('#collection_chooser_collection_id').val();

This comment has been minimized.

@davojta

davojta Oct 28, 2018

Contributor

looks like in wagtain js code you need to user camelCase for var names
@thibaudcolas @jjanssen what do you think ?

This comment has been minimized.

@m1kola

m1kola Oct 28, 2018

Member

If this is the only tweak - I can do this in merge

This comment has been minimized.

@nott

nott Oct 28, 2018

Contributor

Nice catch

Fixed in e28a6f2

This comment has been minimized.

@jjanssen

jjanssen Oct 28, 2018

Member

Honestly it wouldn't surprise me if there would be a mix & match of all sorts of notations within our current javascript definitions. As our initial javascript linting accidentally got disabled some time ago (#2797).

Looking at our more recent work regarding the integration of the Explorer menu and Draftail in React we enforce camelCasing in our variable names. But since this is a bit of legacy I'm ok with leaving this as is.

@m1kola

m1kola approved these changes Oct 28, 2018 edited

Looks good from the BE perspective. I'm happy to merge this after the FE review.

@jjanssen could, please, you review this?

@m1kola m1kola requested a review from jjanssen Oct 28, 2018

@jjanssen

This comment has been minimized.

Member

jjanssen commented Oct 28, 2018

Just gave it a testdrive in Chrome latest, Firefox ESR and Internet Explorer 11.
If the back-end is approved upon, I'm allright with merging this.

m1kola added a commit that referenced this pull request Oct 28, 2018

@m1kola

This comment has been minimized.

Member

m1kola commented Oct 28, 2018

Rebased and merged on top of the master branch f1148e7...bfbdc21. Release notes added in bfc4175.

Thanks, Stas :)

@m1kola m1kola closed this Oct 28, 2018

@nott nott deleted the nott:documents-chooser-helper-text branch Oct 28, 2018

@m1kola m1kola added this to the 2.4 milestone Oct 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment