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

making the external assets form optional #2602

Merged
merged 5 commits into from
Mar 5, 2020

Conversation

jcamejo
Copy link
Contributor

@jcamejo jcamejo commented Feb 26, 2020

Hi @artf i hope you are having a nice day.

What do you think of this small improvement? I just got a use case where i needed to have this option and i think is a good idea.

Thanks!

@artf
Copy link
Member

artf commented Mar 3, 2020

Yeah cool, seems like a good idea 👍, but can you check the failing tests, please?

@jcamejo
Copy link
Contributor Author

jcamejo commented Mar 3, 2020

Sorry for leaving that behind, the tests are added. There are some redeclarations of the test object to make clear that the view is being initialized with the correct config

@@ -56,7 +56,7 @@ export default Backbone.View.extend({
const url = input.value.trim();
const handleAdd = this.config.handleAdd;

if (!url) {
if (!url || !input) {
Copy link
Member

Choose a reason for hiding this comment

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

mmm this !input check doesn't make sense here, it that case it would crash at input.value.trim()

Copy link
Member

@artf artf left a comment

Choose a reason for hiding this comment

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

Check that !input check, I think it is in the wrong place. Probably input && input.value.trim() would be enough

@jcamejo
Copy link
Contributor Author

jcamejo commented Mar 4, 2020

Absolutely right, it's changed.

@jcamejo
Copy link
Contributor Author

jcamejo commented Mar 4, 2020

I'm not sure if this failure has something to do with the change, it does not look like it. I've merged the dev branch to have the latest state. Should i check some other module that I'm not aware of?

Thanks again for your assistance.

@artf
Copy link
Member

artf commented Mar 5, 2020

I'm not sure if this failure has something to do with the change, it does not look like it

Nope, my fault 😬

Thanks for the contribution Juan

@artf artf merged commit 626adff into GrapesJS:dev Mar 5, 2020
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.

2 participants