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

FEATURE: upload dialog with configurable additional property fields #165

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

andrehoffmann30
Copy link

@andrehoffmann30 andrehoffmann30 commented Dec 20, 2022

What I did
I adjusted the upload dialog. It is now possible to configure additonal property filed sto collect the information while uploading a news file. The new new upload dialog is used in the backend module and ind the content area.

How I did it
For the backend module I adjusted the existing upload dialog to respect the new configuration options and handel the rendering and file upload logic accordinglly. For the Content area i added a MediaUploadScreen as a scondary editor to render and handle the file upload like in the bakcend module. For thsi to function also an adjustment of the image editor is needed (see neos/neos-ui#3302 ).

How to verify it
Configure the fileds according to the example in the read me, than the uplaod dialog should render the new additional fields and bahave as configured.

image

@andrehoffmann30 andrehoffmann30 changed the title Feature/upload dialog with copyright notice FEATURE: upload dialog with configurable additional property fields Dec 20, 2022
@andrehoffmann30 andrehoffmann30 force-pushed the feature/upload-dialog-with-copyright-notice branch from 975e671 to 79ad94d Compare December 21, 2022 11:30
@crydotsnake crydotsnake added the Feature A new feature label Dec 24, 2022
@Sebobo Sebobo closed this Feb 10, 2023
@Sebobo Sebobo reopened this Feb 10, 2023
@andrehoffmann30 andrehoffmann30 force-pushed the feature/upload-dialog-with-copyright-notice branch from 79ad94d to 277cc45 Compare February 10, 2023 14:38
@Sebobo Sebobo self-requested a review October 13, 2023 11:03
@andrehoffmann30 andrehoffmann30 force-pushed the feature/upload-dialog-with-copyright-notice branch from c98fbd6 to 94ddbc0 Compare November 10, 2023 15:07
@andrehoffmann30 andrehoffmann30 force-pushed the feature/upload-dialog-with-copyright-notice branch from 94ddbc0 to 2b1f31f Compare November 13, 2023 07:37
Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

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

This cannot be merged without the necessary core change, which I assume could now target 9.0 or 9.1, but the Media.UI itself is not compatible with 9.x yet.

Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

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

Thanks for this huge change.
But several adjustments are necessary which will also make this change cleaner.

I haven't checked everything yet, so I will add further comments when I see issues.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to not have any compiled files in the PRs as they add much data to the repo.

Copy link
Author

Choose a reason for hiding this comment

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

I understand your opinion, but how else should I make the changes functionally available. Without the built files, you can't simply check out the branch and use it in your project. In order to be able to use the changes easily, I think it makes sense to commit the built files as well. Still, I am open to suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

If you use the branch actively you should have a separate branch with the bundles as the default behaviour often is also to delete branches on merge which would break your project.
Then you update your "production" branch from this one on changes and rebuild.

In this PR you committed the bundles several times which are quite a bit of size to the repo. It's a bit annoying but I see no better way and I've been looking for a better way since quite a while.

@andrehoffmann30
Copy link
Author

Hallo @Sebobo, thank you very much for your feedback. I adjust some of my changes and left additional explanations to your comments. Feel free to give me further feedback. Thank you.

@Sebobo
Copy link
Member

Sebobo commented Nov 22, 2023

Hi Andre, thx!
Will take me some days again to go through things :)

Copy link
Member

Choose a reason for hiding this comment

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

If you use the branch actively you should have a separate branch with the bundles as the default behaviour often is also to delete branches on merge which would break your project.
Then you update your "production" branch from this one on changes and rebuild.

In this PR you committed the bundles several times which are quite a bit of size to the repo. It's a bit annoying but I see no better way and I've been looking for a better way since quite a while.

…or NewAssetUploaddialog, adjust css and logic in FIlePreview
@andrehoffmann30
Copy link
Author

Hi @Sebobo I adjusted the components and styling as you suggested. I only committed my code changes, but no builds to not further increase the size of my pr unnecessarily. I also did not rebase to the latest main. Please take a look at the changes and let me know, if there are still problems. Please contact me as soon as you are ready to merge my changes, so that I can rebase my branch. Please also contact me if you have any further questions. Thank you very much.

@Sebobo
Copy link
Member

Sebobo commented Feb 20, 2024

@andrehoffmann30 thank your! I plan to review again end of this week or start of next.

@andrehoffmann30
Copy link
Author

Hi @Sebobo , have you had time to check my latest adjustments? Please let me know if I can help you in any way. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature A new feature
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

3 participants