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

Use weight field type for field_weight (Issue-1262) #8

Closed
wants to merge 6 commits into from
Closed

Use weight field type for field_weight (Issue-1262) #8

wants to merge 6 commits into from

Conversation

seth-shaw-unlv
Copy link
Contributor

@seth-shaw-unlv seth-shaw-unlv commented Sep 10, 2019

DO NOT MERGE!

We are still weighing the merits of using the weight module over using the standard integer field type with our own drag-n-drop support.

GitHub Issue: Islandora/documentation/issues/1262

What does this Pull Request do?

Changes field_weight from an integer field type to the weight module's weight field type. Adds the weight module as a requirement and updates the rdf mapping. Adds a new "Re-order Members" view and associated tab to provide a drag-n-drop item re-order UI.

What's new?

  • Changes field_weight such that its type changes from integer to weight.
  • Added rdf mapping for field_weight as schema:position; adds the schema:CreativeWork type; adds a re-order members view/tab.
  • Does this change require documentation to be updated? I don't think we had weight documented yet.
  • Does this change add any new dependencies? The weight module.
  • Does this change require any other modifications to be made to the repository
    (ie. Regeneration activity, etc.)? This requires you to delete the existing weight field before doing a feature import. This will remove any data you have in there, but since the field as added earlier today, we should be okay.
  • Could this change impact execution of existing code? No.

How should this be tested?

  • If you have an existing field_weight: delete your field_weight.
  • apply the PR
  • Import the Islandora_defaults feature drush fim -y islandora_defaults
  • Make an item with a few members.
  • Go to the item's "Re-order members" tab and re-order the members as desired. Click "Save" to save the re-order. Optionally, you can click the "Show row weights" link to explicitly set weight values. Screen Shot 2019-09-11 at 8 52 19 AM Note: this ordering is not used on the "Members" tab as that tab is contained in the islandora module that is unaware of field_weight; but you can add the sort manually.

Additional Notes:

This setup means that all repository items will have a default field_weight value of '0' which will persist to Fedora as a schema:position value.

Interested parties

@dannylamb, @dbernstein, @Islandora-CLAW/committers

@manez
Copy link
Member

manez commented Sep 11, 2019

As noted in the call I think I'd prefer to have a separate content type for paged content versus putting cruft into Fedora on items that don't need to have a weight, but if this can be done with context instead, all the better 👍

@seth-shaw-unlv
Copy link
Contributor Author

Assuming we do use a second content type, this should probably be a separate module that can be enabled if desired. Also, what fields would we want on that "Repository Object Child"? The full default set we have on "Repository Object"? That is going to create a lot of extra field configs that we will have to keep up in parallel. OR do we assume that child parts will have a sub-set; e.g. just title, date, description, typed agents, and subjects? I guess that is a MIG question. (Tagging @rtilla1 and @mbolam.)

@dannylamb
Copy link
Contributor

dannylamb commented Sep 11, 2019

All those duplicated fields were the motivation behind doing it with one content type and tags, but as I said in the meeting, we can always make more/other content types. We're not excluded from having more just because we went with one to simplify things at that point in time.

I'm happy with leaving it up to users to choose. We can implement whatever makes the most sense to people.

@seth-shaw-unlv
Copy link
Contributor Author

Honestly, I'm getting really tempted to make a custom FieldWidget that allows a null value entry. The existing widget is suuuuper simple. We would use it locally but then also contribute it back to the module. Should they ever decide to merge the contribution we can remove ours.

@dannylamb
Copy link
Contributor

@seth-shaw-unlv That'd be easy enough to do. I'm still thinking about what a new content type would entail, though. Do we ever really describe individual pages? If we can drop all the descriptive metadata it might not be too bad of a solution.

@seth-shaw-unlv
Copy link
Contributor Author

The more I look at this the more I want to cringe and avert my eyes from the RDF.

Making our own widget with a 'N/A' (empty) value was super simple as I expected. I set 'N/A' as the default value and everything works beautifully. HOWEVER, when editing a page manually it means scrolling past 1000 negative values before you get to positive ones. It also means that if you use the drag-n-drop ALL of the children get new weights starting from the smallest value up (e.g. 'Page 1' would get the weight of -1000 and 'Page 2' gets -999 as currently configured).

Sure this all works and the Schema docs don't state a preference for zero-based or positive only values, but any human looking at these values in the JSON-LD/Fedora RDF are going to scratch their heads at all the child objects having negative values (cause how many objects are going to have > 1001 children?).

@seth-shaw-unlv
Copy link
Contributor Author

Do we ever really describe individual pages? If we can drop all the descriptive metadata it might not be too bad of a solution.

True, we can leave off all the added fields by default. Repository managers can add more if they wish. We'll still have all of these negative weight values, though.

@dannylamb
Copy link
Contributor

Yeah, the negative weight thing does seem pretty weird... I understand it could be useful to pop in a -1 to bubble something to the top, but negative pages don't make sense.

@seth-shaw-unlv
Copy link
Contributor Author

Okay, so let's hold off on this one until I poke at doing a straight-up port of the drag-n-drop from weight to integer. Looking at the code tells me it won't be too difficult.

Later we can issue a feature request to support a positive-weight option to the weight module, perhaps contribute the necessary updates, and (if it ever gets merged) migrate at that point. The upgrade path from integer to weight when/if that time comes would be pretty simple because the weight field type is a thin wrapper around integer (integer value + range setting).

@manez
Copy link
Member

manez commented Sep 12, 2019

Negative weight is weird from a numbering pages perspective, but it's very, very Drupal. I am way more Drupal front-end builder than metadata librarian (MLIS notwithstanding), but I didn't blink when I was testing it, other than to worry that getting up to a range of 2000 digits in a drop down is going to be ** a thing**.

Am I following this conversation right in reading that @seth-shaw-unlv is going to make a custom widget to re-order instead of using the weight module as it stands?

@seth-shaw-unlv
Copy link
Contributor Author

@manez, I'm going to give it a try. I agree that the way weight works is very, very Drupal, but I'm concerned that this Drupal-ism may be off-putting to Islandora users and RDF users (and possibly Schema.org consumers; but that remains to be seen). I'll give it a go and see what happens.

@manez
Copy link
Member

manez commented Sep 12, 2019

@seth-shaw-unlv 👍 👍 👍 entirely agree, it sounds like much better Islandora UX. Just making sure I was actually following 😄

@dannylamb
Copy link
Contributor

@seth-shaw-unlv Have you checked out https://www.drupal.org/project/draggableviews ?

@seth-shaw-unlv
Copy link
Contributor Author

Didn’t know about it. Thanks for the tip! In an EaaSI webinar at the moment but I will give it a whirl afterwards.

@seth-shaw-unlv
Copy link
Contributor Author

And this is why you close notifications during webinars.... a brief scan of the draggabletables code shows that they are creating a separate view/entity/weight table. It doesn’t use or update the weight value stored on the node. 👎

@dannylamb
Copy link
Contributor

Superseded by Islandora/documentation#171

@dannylamb dannylamb closed this Sep 20, 2019
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.

3 participants