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

resurrect clj-pdf based pdf rendering #2061

Merged
merged 18 commits into from Mar 23, 2020
Merged

resurrect clj-pdf based pdf rendering #2061

merged 18 commits into from Mar 23, 2020

Conversation

@opqdonut
Copy link
Collaborator

opqdonut commented Mar 18, 2020

for #2053

Definition of Done / Review checklist

Reviewability

  • link to issue
  • consider adding screenshots for ease of review

API

  • API is backwards compatible or completely new

Documentation

  • update changelog if necessary
  • documentation at least for config options (i.e. docs folder)

Testing

  • complex logic is unit tested
  • valuable features are integration / browser / acceptance tested automatically

Follow-up

  • no critical TODOs left to implement
opqdonut added 10 commits Mar 18, 2020
- path is now /api/applications/:id/experimental/pdf
- ns is rems.experimental.pdf
This reverts commit 174ac4d, reversing
changes made to bbd7fc9.
clj-pdf is very inconsistent with where it allows lists
- include all members of application
- nicer user formatting
- less verbose event table
- show application title
- show licenses before fields
- nicer formatting for fields
@opqdonut

This comment has been minimized.

Copy link
Collaborator Author

opqdonut commented Mar 18, 2020

Here's what the PDF looks like now
example-application.pdf

@opqdonut opqdonut force-pushed the pdf-2053 branch from c5fe94d to f53b9b5 Mar 18, 2020
Copy link
Collaborator

Macroz left a comment

Some suggestions

  • Label should perhaps be plaintext
  • Header does not really show much
  • Both kinds of options show key not label
  • Printing date?
  • Events should be a the end?
  • Order of events is reverse of UI
  • Latest activity could be shown with the state like in UI.
  • The margin after a heading is inconsistent. Almost none with Title and Applicants but some with Events and Application blocks.
([:paragraph "Google license"]
[:paragraph "Text license"]))]
(with-language :en
#(#'pdf/render-application (applications/get-application handler application-id))))))))

This comment has been minimized.

Copy link
@Macroz

Macroz Mar 19, 2020

Collaborator

Hmm?

This comment has been minimized.

Copy link
@opqdonut

opqdonut Mar 19, 2020

Author Collaborator

with-language is a function, not a macro

@opqdonut

This comment has been minimized.

Copy link
Collaborator Author

opqdonut commented Mar 19, 2020

Some suggestions

  • Label should perhaps be plaintext
  • Header does not really show much

Do you mean the label and header field types here?

  • Both kinds of options show key not label

Will do

  • Printing date?

You mean the date the pdf was generated? Good idea.

  • Events should be a the end?

Yeah that's a good idea

  • Order of events is reverse of UI

In a hardcopy it's more natural to show the events in chronological order IMO.

  • Latest activity could be shown with the state like in UI.

Let's add it if there's a request.

  • The margin after a heading is inconsistent. Almost none with Title and Applicants but some with Events and Application blocks.

I'll take a look if something can be done about that.

@opqdonut opqdonut force-pushed the pdf-2053 branch from 34a4cdf to 61aed9f Mar 19, 2020
@opqdonut

This comment has been minimized.

Copy link
Collaborator Author

opqdonut commented Mar 19, 2020

here's a fresh example:
example-application.pdf

@Macroz
Macroz approved these changes Mar 23, 2020
Copy link
Collaborator

Macroz left a comment

I think if you check the possibility of adding some spacing between sections, then this is good to merge.

@opqdonut

This comment has been minimized.

Copy link
Collaborator Author

opqdonut commented Mar 23, 2020

now with more spacing:
example-application.pdf

@opqdonut opqdonut merged commit 9e6d0ff into master Mar 23, 2020
6 checks passed
6 checks passed
WIP Ready for review
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: doo Your tests passed on CircleCI!
Details
ci/circleci: ok Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: without-db Your tests passed on CircleCI!
Details
@opqdonut opqdonut deleted the pdf-2053 branch Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.