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

Added address info to xls export #113

Conversation

philipp-ullmann
Copy link

  • Added address info to xls export of contacts, leads and accounts.
  • Moved rendering of xls export into a view with xml builder.
  • Created layout file for xls export.
  • Added internationalization to xls export of contacts, leads and accounts.

Feedback to existing export:

  • It's a better practice to move view logic into views and out of models.
  • xls and csv export is too generic.
  • I would avoid to add methods to a core ruby class (Array). This could lead to curious bugs, if for example a gem also adds methods with the same name.

@ndbroadbent
Copy link
Contributor

Hi, this looks great, thanks!

I was wondering if I could request that when you submit pull requests, could you please select "merge into the 'master' branch"?

i.e. your pull requests look like this:

merge 6 commits into fatfreecrm:9a94d39 from create-mediadesign:41c39ba

But it would be more helpful if they could be:

merge 6 commits into fatfreecrm:master ...

(Because otherwise, Github tells us "This pull request cannot be automatically merged.")

Thanks very much for your contributions!

@philipp-ullmann
Copy link
Author

Hi,

next time I will try to merge into the master branch. But I don't know if it is possible, because I must select a commit range, in order to exclude commits not suitable for the base project. But I will try!

@ndbroadbent
Copy link
Contributor

Ah, I see! We usually solve this problem by creating a branch, and only adding the relevant commits to that branch. You can even use the git cherry-pick feature for this.

I will try to merge in your changes ASAP (a lot has changed in FFCRM recently, so the merge might be a little bit difficult)

@steveyken
Copy link
Member

@ndbroadbent Did this commit make it in yet? Looking forward to having it :) Thanks @create-philipp-ullmann

@philipp-ullmann
Copy link
Author

Next week.

@philipp-ullmann
Copy link
Author

Manual merge into master branch

@steveyken
Copy link
Member

thanks! and useful for showing how builder works.

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.

None yet

3 participants