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

Bitstream format registries #455

Merged
merged 7 commits into from Aug 30, 2019

Conversation

YanaDePauw
Copy link
Contributor

This PR will add the functionality necessary to create, update and delete bitstream formats available in the admin registries.

The bitstream-format overview has been updated to allow deletion of formats by selecting formats using checkboxes and clicking a delete button.
A link has been added on the page to create new bitstream formats that will redirect the user to a form where all bitstream format info can be filled in.
Furthermore, the available bitstream formats on the overview page can be clicked which will redirect the user to an edit form similar to the create form that can be used to update the bitstream format.

 Conflicts:
	src/app/+admin/admin-registries/bitstream-formats/bitstream-formats.component.spec.ts
	src/app/core/core.module.ts
	src/app/core/registry/registry-bitstreamformats-response.model.ts
	src/app/core/registry/registry.service.ts
	src/app/core/shared/bitstream-format.model.ts
Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @YanaDePauw!

Everything works as it should, but I made a few small inline comments.

Also a usability note: when adding an extension I wondered briefly whether I should prefix it with a point or not. Perhaps add a help text to clarify that you shouldn't.

Finally, if try to use a bitstream format short description that already exists, you are allowed to submit it. When you do, you get an error message saying that it failed, without a reason why.

It would be nice if we had a validator to check whether a short description is unique as you're typing, but I realize there is currently no rest endpoint that would allow you to write that validator. So I'll bring it up in the next meeting, and if we agree that it's worth the effort, that validator can be created in a separate PR.

Change padding into margin
Change 'Create link' to a button
Add placeholder to extension field
Fix pagination
Copy link
Contributor Author

@YanaDePauw YanaDePauw left a comment

Choose a reason for hiding this comment

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

@artlowel I made a change to the extension field to use a placeholder with some additional info about the extension.
The reason for this was that I could not add a hint easily to the repeatable field. When I added a hint, it would be repeated for every input box which looked worse than using a placeholder.

Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @YanaDePauw
LGTM

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Tested this today, and it works great. Thanks @YanaDePauw !

One tiny improvement to think about (for a future PR likely)... the order of formats in the registry seems "random" (I'm guessing they are ordered by internal ID perhaps?). The randomish ordering makes it very difficult to local a particular bitstream format to edit/update (you basically have to keep paging till you see it).

So, ideally, at some point, there'd either be a way to reorder the entries (perhaps by name by default), or a way to search within them (to find the one you want).

Nonetheless, I think this is a good initial version of the registry. So, I'm fine with this PR being merged as-is.

@artlowel
Copy link
Member

@tdonohue It doesn't seem like the rest endpoint supports any way to sort the bitstreamformats. We can't add sorting client side, since the rest response is paginated.

So I'll merge this PR now as is

@artlowel artlowel merged commit ee56de9 into DSpace:master Aug 30, 2019
@benbosman benbosman deleted the Bitstream-format-registries branch September 11, 2020 07:44
@tdonohue tdonohue added this to the 7.0beta1 milestone Jan 26, 2021
4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request Feb 8, 2023
kosarko pushed a commit to kosarko/dspace-angular that referenced this pull request Apr 3, 2024
try to append server/ to test gh action
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.

None yet

3 participants