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

docs(dotnet): introduce separate csharp viewport option #6198

Merged
merged 1 commit into from
May 5, 2021
Merged

docs(dotnet): introduce separate csharp viewport option #6198

merged 1 commit into from
May 5, 2021

Conversation

avodovnik
Copy link
Contributor

Due to our handling of null, we want to be explicit in the docs, that the user needs to pass in a special value for undefined.

@avodovnik
Copy link
Contributor Author

The other option here, would be to introduce code examples for all of the languages to demonstrate how to disable the viewport, but that feels like a higher overhead than the proposed solution.

Now, if only we had language-specific notes >:-]

@pavelfeldman
Copy link
Member

I'd either follow js or python, no need for unique approach in c#

@pavelfeldman
Copy link
Member

@pavelfeldman
Copy link
Member

Looks abandoned.

@avodovnik avodovnik reopened this May 3, 2021
@avodovnik
Copy link
Contributor Author

@pfeldman In C#, the approach of using a constant is more familiar than using a separate parameter. We also have a ViewportSize.Default for that same reason. The better option would be to have the default value be set by the generator, but that goes against the other options where we say if it's null, the value is default from the driver.

For that reason, I'd like to keep it this way.

@avodovnik avodovnik requested a review from yury-s May 4, 2021 09:38
@kblok
Copy link
Contributor

kblok commented May 4, 2021

@pavelfeldman @avodovnik can we land this PR? :)

docs/src/api/params.md Outdated Show resolved Hide resolved
docs/src/api/params.md Outdated Show resolved Hide resolved
@yury-s yury-s merged commit 90de864 into microsoft:master May 5, 2021
- `width` <[int]> page width in pixels.
- `height` <[int]> page height in pixels.

Emulates consistent viewport for each page. Defaults to an 1280x720 viewport. Use `ViewportSize.NoViewport` to disable the default viewport.
Copy link
Member

Choose a reason for hiding this comment

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

An afterthought: maybe rename it to NoDefaultViewport for clarity?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not strong to say no, but I don't know if it will bring more clarity. If you start typing ViewportSize. as a static call, you kinda know where you are going to... you are going to No(something).

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.

4 participants