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/removable files #291

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mrdj07
Copy link

@mrdj07 mrdj07 commented Aug 31, 2021

I needed to fix the remove image problem. I had in mind a solution of flattening the fields of the layout in order to let the Deletion Controller do it's job, but this pull request had it laid down pretty nicely.

I based my branch from master, cleaned up some stuff I found unnecessary in the initial PR, then found the following issues and fixed them:

  • The Original PR worked to delete the files, tough this was not the correct way of dealing with the model, this way I let Nova know what fields to update and it does the job.
  • The FormField did not update the last version timestamp after the deletion of the file, as the original FormFileField does. So I added the needed events to be caught by the FormPanel.
  • The Add Layout button was gone: The field limit counter was buggy. Returned a "NaN" when no limit was set. Added validation for both the original dropdown and the searchfield.
  • The required Laravel Version was added by previous commits, I added version 8, since this is the version I'm working with, and I've encountered no problems at all.

I also reformatted all of the PHP files in PSR-2.

@toonvandenbos
Copy link
Member

Hi @mrdj07,

Thanks for your PR ! At first glance, it looks good to me.

You mentioned some issues with the limit counter, could you please check if your PR needs changes since I merged PR #292 (adding some extra limit functionalities) ? Thanks !

@mrdj07
Copy link
Author

mrdj07 commented Sep 2, 2021

Seems like the limit issues were indeed fixed.

Though now my PR is in conflict, I'll fix it asap.

@mrdj07
Copy link
Author

mrdj07 commented Sep 2, 2021

@Nyratas The PR is up to date !

@chrisneal
Copy link
Contributor

@mrdj07 Thanks for taking my PR and improving it so much!

@Nyratas Any idea when this PR could get merged?

@mrdj07
Copy link
Author

mrdj07 commented Sep 8, 2021

After my lastest merge I had a bad conflict resolution resulting in an infinite loop, it's now fixed.

@chrisneal
Copy link
Contributor

@Nyratas This is quite a big issue for us currently. Could you spend sometime reviewing this PR please?

@jasperlanting10
Copy link

Any information on when this PR going to get merged?

@chrisneal
Copy link
Contributor

@Nyratas @voidgraphics Please review this PR. If have no time or are you're no longer willing to maintain this, then please pass it over to someone who can.

@voidgraphics
Copy link
Member

@chrisneal We're trying to do that -> #326

@mrdj07
Copy link
Author

mrdj07 commented May 5, 2022

Updated

@voidgraphics
Copy link
Member

Sorry for the wait on this, I will do my best to get to it this week. Thanks again for submitting this @mrdj07

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.

5 participants