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

Check for relative paths #247

Merged
merged 14 commits into from
Jul 6, 2019
Merged

Conversation

eikaramba
Copy link
Contributor

@eikaramba eikaramba commented May 13, 2018

As mentioned in #246, this idea is based on the fact that relative paths are normally not really supported in email clients(enlighten me if you think so. Maybe base64 encoded data, but the support ist quite bad). Thus, it might be nice to inform the user about this after exporting(i don't want to block the export action). This PR is WIP and open for discussion.

Works like this:
demo

  • Unfortunately you often have variables in your href or src like {{my_link}} which are meant to be replaced with a link in your backend system. This will be falsely detected as a relative path.
  • The alert is now not going away automatically. i changed the alert api in order to support this.

Copy link
Contributor

@meriadec meriadec left a comment

Choose a reason for hiding this comment

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

hey @eikaramba , thx for the PR, can you make it pass CI + update your code following feedbacks?

src/components/Alerts/index.js Outdated Show resolved Hide resolved
src/reducers/alerts.js Outdated Show resolved Hide resolved
src/pages/Project/index.js Outdated Show resolved Hide resolved
@eikaramba
Copy link
Contributor Author

yes i will try my best :) sry first time using react and electron. I will try to adress all your issues

@eikaramba
Copy link
Contributor Author

mhm somehow prettier command is failing, need to see another day what is causing that

@eikaramba
Copy link
Contributor Author

added some form of tooltip for the description

demo

@eikaramba
Copy link
Contributor Author

is there anything that i can do to make this PR succeed? Or do you not know exactly whether this feature should be merged or not?

@iRyusa
Copy link
Member

iRyusa commented Jul 2, 2019

It's a bit late here... I'm sorry.

I would like to merge this into master @eikaramba can you just fix the conflict ?

@iRyusa
Copy link
Member

iRyusa commented Jul 3, 2019

Looks like you still have some issues with prettier, sorry about this, can you run it on listed file ?

@eikaramba
Copy link
Contributor Author

yes i saw this, just hadn't had the time. will do this on the weekend

@eikaramba
Copy link
Contributor Author

unfortunately both prettier and prettier:check run without any error on my pc. that is very strange, it seems there is nothing wrong here. travis does not include any clue what could be the problem.

@iRyusa
Copy link
Member

iRyusa commented Jul 5, 2019 via email

@eikaramba
Copy link
Contributor Author

i saw that, but unfortunately it runs fine locally and i don't see anything wrong with it, when i look manually into those files. i even tried to manually execute the prettiert command for all 3 files in VS Code but there was no change.

@meriadec
Copy link
Contributor

meriadec commented Jul 6, 2019

updated. that's strange, maybe check the locally installed prettier version. on my side it's 1.18.2

LGTM thanks for the PR and sry for the delay 😅

@meriadec meriadec merged commit eb2d208 into mjmlio:master Jul 6, 2019
@eikaramba
Copy link
Contributor Author

will do, that is very strange. but thank you very much

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.

3 participants