Skip to content

Conversation

wardi
Copy link
Contributor

@wardi wardi commented Jul 5, 2021

Fixes #6226

Proposed fixes:

Create extensible resource upload snippet with reduced JS, separate from the image upload macro. Allows easily adding new buttons to the the "Data: [upload] [link]" menu part of the resource form.

Behavior changes

  • The upload button no longer immediately opens your browser file dialog, a second click on the revealed file upload widget is now required.
  • clearing existing uploads keeps the file upload section selected instead of resetting to the "Data: [upload] [link]" menu. A second click on the "Remove" button will return to the menu. This could be changed to old behavior with a couple lines of JS if reviewer thinks appropriate.
  • Remove buttons are now positioned to the right of the field labels instead of within the edit box for uploads and links. This was the easier implementation and I think it improves usability to have a margin between different interactive elements

That this PR includes changes from #6218 which should be reviewed first

Features:

  • includes tests covering changes
  • includes updated documentation
  • includes user-visible changes
  • includes API changes
  • includes bugfix for possible backport

@wardi wardi changed the title 6226 resource upload snippet Resource upload field snippet Jul 5, 2021
@wardi wardi marked this pull request as ready for review July 8, 2021 19:36
@wardi
Copy link
Contributor Author

wardi commented Jul 14, 2021

buttons
This is how the new snippet can be extended with a new, functioning button with just an extended template: 272508b

@amercader amercader mentioned this pull request Sep 16, 2021
5 tasks
@amercader amercader self-assigned this Oct 5, 2021
Copy link
Member

@amercader amercader left a comment

Choose a reason for hiding this comment

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

@wardi this looks good so far, I added some comments and questions

To your behavior change comments:

The upload button no longer immediately opens your browser file dialog, a second click on the revealed file upload widget is now required.

See comment here. I like to get the file picker after the first click

clearing existing uploads keeps the file upload section selected instead of resetting to the "Data: [upload] [link]" menu. A second click on the "Remove" button will return to the menu. This could be changed to old behavior with a couple lines of JS if reviewer thinks appropriate.

I don't think that's a big deal. Actually if you have an existing upload you are more likely to replace with another upload than a URL so it makes sense.

Remove buttons are now positioned to the right of the field labels instead of within the edit box for uploads and links. This was the easier implementation and I think it improves usability to have a margin between different interactive elements

I personally like the "Remove" buttons inline with the input box. I think it makes clear what you are clearing

@@ -0,0 +1,114 @@
{#
Copy link
Member

Choose a reason for hiding this comment

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

This snippet is not the easiest to read but is definitely easier to follow than the old JS module one.

Just to see if I'm reading it right is this how you would add your own button (in your own resource_upload_field.html)?

{% ckan_extends %}

{% block url_type_select %}
  {{ super() }}
  {# Add the button here #}
{% endblock %}

{% block url_type_fields %}
  {{ super() }}
  {# Add the actual field here #}
{% endblock %}

And with more detail:

{% ckan_extends %}

{% block url_type_select %}
  {{ super() }}

  {# Add the button here #}
  <button type="button" class="btn btn-default" id="resource-import-button"
    title="{{ _('Some other way of adding data') }}"
    onclick="
      document.getElementById('resource-import-link').checked = true;
      document.getElementById('field-resource-import').focus();
    "><i class="fa fa-globe"></i>{{ _('Import Data') }}</button>

{% endblock %}

{% block url_type_fields %}
  {{ super() }}
  {# Add the actual field here #}
  {% is_import = data.get('import') %}
  <input type="radio" id="resource-url-import" name="url_type" value="import" {{
    'checked' if is_import else '' }}>
  <div class="select-type">
      {% block link_controls %}
        {{ remove_button(
          js="$('#field-resource-import').val('')") }}
        {{ form.input(
          'import',
          label= _('Import URL'),
          id='field-resource-import',
          type='url',
          placeholder=placeholder,
          value='' if is_upload or is_url else data.get('import'),
          error=errors.get('import'),
          classes=['control-full']) }}
      {% endblock %}
    </div>

{% endblock %}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I provided an example for adding a button above #6227 (comment)

wardi and others added 3 commits October 15, 2021 09:55
Co-authored-by: Adrià Mercader <amercadero@gmail.com>
Co-authored-by: Adrià Mercader <amercadero@gmail.com>
Co-authored-by: Adrià Mercader <amercadero@gmail.com>
@wardi
Copy link
Contributor Author

wardi commented Oct 15, 2021

Re: the remove button position change, it was also tricky to write the CSS to have the remove button overlap with the input box, and overlapping wouldn't make sense for all url_type inputs so it will make the code more complicated. Can you think of a way to manage this per url_type? raw css inside the templates?

@amercader
Copy link
Member

Re: the remove button position change, it was also tricky to write the CSS to have the remove button overlap with the input box, and overlapping wouldn't make sense for all url_type inputs so it will make the code more complicated. Can you think of a way to manage this per url_type? raw css inside the templates?

These CSS tweaks bring the button aligned as we have before:

diff --git a/ckan/public/base/scss/_forms.scss b/ckan/public/base/scss/_forms.scss
index 415278715..788a5cd69 100644
--- a/ckan/public/base/scss/_forms.scss
+++ b/ckan/public/base/scss/_forms.scss
@@ -779,10 +779,10 @@ select[data-module="autocomplete"] {
     }
 
     .btn-remove-url {
-        // "Remove" buttons float right of the select-type div labels
         position: absolute;
         margin-right: 0;
-        top: 0;
+        margin-top: 2px;
+        top: $grid-gutter-width;
         right: $grid-gutter-width / 6;
         padding: 0 12px;
         border-radius: 100px;

Screenshot 2021-10-29 at 10-56-50 Edit - fdsf - Test add resource buttons - CKAN

Screenshot 2021-10-29 at 10-54-36 Edit - fdsf - Test add resource buttons - CKAN

I think that the aligned buttons make sense for all input field based url_types and will keep the existing UI, and if needed extensions can either override the CSS or not include the remove_button() macro at all and implement their own thing.

What do you think?

@amercader
Copy link
Member

I see that with these changes resources that are created with Links have a url_type=link metadata field. These used to have url_type=None before so we probably should document that in the changelog.

I assume that this will only happen automatically if users are creating the resource from the UI (because the form field is sent). If external resources are created via the API the url_type will be None unless the user explicitly provides it. Trying to enforce this at the validation level seems problematic as it could clash with other custom types like datastore, etc, but maybe we should document it in the resource_create's url_type docstring?

@wardi
Copy link
Contributor Author

wardi commented Jan 18, 2022

@amercader these should be fixed now

@amercader amercader merged commit c5f479f into master Jan 19, 2022
@amercader amercader deleted the 6226-resource-upload-snippet branch January 19, 2022 14:12
@amercader
Copy link
Member

Thanks @wardi! merging now

@amercader amercader mentioned this pull request Jan 19, 2022
5 tasks
amercader added a commit to frictionlessdata/ckan-uploader that referenced this pull request Mar 3, 2023
Copy the form elements from upstream CKAN. We use the form fields in
CKAN 2.10, which were refactored to simplify them and drop the infamous
image-upload.js module in ckan/ckan#6227.

The markup is clearly separated in three blocks:
1. Upload/Link buttons
2. File input + Remove button
3. URL input + Remove button

The style classes have been prefixed to avoid clashes with the other
`image-upload` field we have in the form (the schema one, which will
eventually be replaced):

    `.resource-upload-field` -> `.ckan-resource-upload-field`
    `.btn-remove-url` -> `.btn-remove-url-upload`

TODO:
* Remove inline JS
* Autopopulate name
* Hook into upload logic
@wardi wardi mentioned this pull request Oct 30, 2023
5 tasks
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.

Resource upload field snippet
3 participants