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

Feat/file upload modal #431

Merged

Conversation

a-crea
Copy link
Contributor

@a-crea a-crea commented Oct 18, 2023

This pull request includes the new file upload language settings modal.

@gappc
Copy link
Collaborator

gappc commented Oct 19, 2023

@a-crea Thank you for this PR

Unfortunately it doesn't satisfy all checks (see https://github.com/noi-techpark/it.bz.opendatahub.databrowser/actions/runs/6563367797/job/17827429525?pr=431) and can therefor not be merged.

I think the problems are related to missing REUSE license headers in new files. Please take a look at other existing files and see how such headers look like. Then, please add the header to all related files.

@gappc gappc mentioned this pull request Oct 19, 2023
@gappc
Copy link
Collaborator

gappc commented Oct 19, 2023

@a-crea I saw in another PR (#432) that the branch was not up-to-date with the current development branch (see #432 (comment)). The same problem seems to appear in this PR.

Please bring your branch up-to-date with the current development branch doing a rebase (or merge if you prefer). It simplifies the code review very much

thx

@gappc
Copy link
Collaborator

gappc commented Oct 31, 2023

@a-crea thank you for this PR

There still seems to be a bug, where I'm not able to add languages (see screenshot)

image

Steps to reproduce:

=> the popup appears, but there is no way to add languages

In my opinion, all languages should be shown such that a user can select languages that were not selected before


There seems to be another (related?) bug, that happens when more than one documents get uploaded. Ifone wants to change the language info without reloading the page, no language is shown at all in the popup

Steps to reproduce:

=> the popup appears, but there is no language shown at all

Please take a look at these issues, thx

@MatteoBiasi
Copy link
Collaborator

@gappc this has been separated in another issue: #438. For this PR this behaviour it's fine.

@gappc
Copy link
Collaborator

gappc commented Oct 31, 2023

@MatteoBiasi I had the impression that it should be possible to add languages and #438 was there to discuss if the language assignment should be modified (e.g. provide delete / add buttons). The main problem with this implementation is, that it is impossible to add languages after an upload was saved.

@sseppi if the behavior implemented in this PR is fine, we can merge it

@sseppi
Copy link
Contributor

sseppi commented Oct 31, 2023

@MatteoBiasi I had the impression that it should be possible to add languages and #438 was there to discuss if the language assignment should be modified (e.g. provide delete / add buttons). The main problem with this implementation is, that it is impossible to add languages after an upload was saved.

@sseppi if the behavior implemented in this PR is fine, we can merge it

@gappc @MatteoBiasi I agree that the behaviour isn't perfet at the moment, but I need to go online as soon as possible with this new feature. So I would suggest to do compromise, go online with this PR and implement as soon as possibel the changes included in the issue #438

@gappc gappc merged commit 31a9ade into noi-techpark:development Oct 31, 2023
4 checks passed
@gappc
Copy link
Collaborator

gappc commented Oct 31, 2023

@sseppi @MatteoBiasi @a-crea merge is done 👍

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.

4 participants