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

adjust Composable function signature to make source mandatory #135

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wiryadev
Copy link

Current Composable function make all the PDF source as nullable and have default value (null). I think there is at least two issues with that:

  1. People can forget to add Pdf source when running the app initially
  2. People can accidentally pass multiple source, and get confused why the other source is showing instead what they originally meant to passed

So i propose change to Composable function signature to make it clear that it needs exactly one PDF source to be laoded.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for contributing to this project. This is your first pull request and we are so glad to have you onboard. We will review the request and get back to you soon. We love your contributions! Join our Discord community here to discuss this PR or ask questions.

@deddyrumapea
Copy link
Contributor

I've proposed a different approach to this problem:

#132

I'm curious about your opinion on this.

Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Jun 20, 2024
@afreakyelf
Copy link
Owner

Hi, Can you please resolve your conflicts?

@wiryadev wiryadev force-pushed the composeable-signature branch from 2d5a540 to 7edd730 Compare January 18, 2025 11:15
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jan 18, 2025
@wiryadev
Copy link
Author

Hi, Can you please resolve your conflicts?

done, and i've fixed the build issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants