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

Issue #189 Print options fix #238

Merged
merged 4 commits into from
Dec 6, 2018
Merged

Issue #189 Print options fix #238

merged 4 commits into from
Dec 6, 2018

Conversation

ameliebernier
Copy link
Contributor

@ameliebernier ameliebernier commented Nov 23, 2018

Please check if the PR fulfills these requirements

What is the current behavior? (You can also link to an open issue here)

Issue #189

What is the new behavior?

Toggle options
Selectbox for orientation
Margins correction

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[ x ] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications:

Other information:

@@ -340,6 +342,8 @@ export class PrintService {
try {
this.addCanvas(doc, canvas, size, margins);
} catch (err) {
alert('2' + err.toString());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this intended or you forgot to remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot, just removed it

Copy link
Collaborator

@cbourget cbourget left a comment

Choose a reason for hiding this comment

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

Code looks nice. I'll try and test it soon.

@cbourget
Copy link
Collaborator

Good work, thank you!

I think it would be better if there was some padding added the bottom of the slide toggle group because, as it is right now, it overlaps with the next field's placeholder.

Also, I think it would look better if the labels were before the slide toggle but I don't have a strong opinion on that matter. It could very well stay like this.

image

@ameliebernier
Copy link
Contributor Author

Somehow padding wasn't applying well outside of the library app. I used margins instead, now looks like this:

image

I prefer the labels after personnally, it looks neater. If they aligned to the right it would be neat too but when the container is large it feels weird to have them to the right.

@cbourget
Copy link
Collaborator

Looks good!

Copy link
Member

@mbarbeau mbarbeau left a comment

Choose a reason for hiding this comment

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

After the changes, its seems good to me. Great work !

mat-checkbox.horizontal-left span {
padding: 0 8px;
mat-slide-toggle ::ng-deep span {
font-size: 100% !important;
Copy link
Member

Choose a reason for hiding this comment

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

try to avoid using the 'important' keyword

@@ -9,7 +9,8 @@ import {
MatInputModule,
MatFormFieldModule,
MatRadioModule,
MatCheckboxModule
MatCheckboxModule,
Copy link
Member

Choose a reason for hiding this comment

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

I think that the MatCheckboxModule and MatRadioModule modules are no longer useful

projects/geo/src/lib/print/shared/print.service.ts Outdated Show resolved Hide resolved
@mbarbeau mbarbeau merged commit 6f8921e into master Dec 6, 2018
@mbarbeau mbarbeau deleted the fixPrintOptions branch December 6, 2018 20:39
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