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

feat: add clipItemOne prop #137

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

zoltan-mihalyi
Copy link
Contributor

Added clipItemOne prop, which enables clipping on itemOne.

Possible considerations:

  • add clipItemTwo prop
  • make clipItemOne true by default (breaking change)

I could'nt start the project and storybook on my windows machine. If it is easier for you, feel free to cherry pick and edit this code instead of accepting the PR.

Copy link

vercel bot commented Mar 29, 2024

Someone is attempting to deploy a commit to a Personal Account owned by @nerdyman on Vercel.

@nerdyman first needs to authorize it.

@nerdyman
Copy link
Owner

nerdyman commented Apr 2, 2024

Awesome, thanks for this @zoltan-mihalyi! I'll try it out and let you know if it's good to go.

Did you run into a specific error with storybook? Others have had problems spinning it up before so I may have missed something in the contrib guide.

@zoltan-mihalyi
Copy link
Contributor Author

Did you run into a specific error with storybook? Others have had problems spinning it up before so I may have missed something in the contrib guide.

On windows, the rm -rf and NODE_ENV=production tsup commands do not work, but there are workarounds.

But after that i got an error:
23:00:39 [vite] Internal server error: Failed to resolve import "react-compare-slider" from "content\stories\00-demos\00-index.stories.tsx". Does the file exist?

@nerdyman nerdyman linked an issue Apr 5, 2024 that may be closed by this pull request
@zoltan-mihalyi
Copy link
Contributor Author

After removing rm commands from scripts:

pnpm run test:storybook
Error: Executable doesn't exist at C:\Users\***\AppData\Local\ms-playwright\chromium-1097\chrome-win\chrome.exe`
pnpm exec playwright install
ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL  Command "playwright" not found

I feel like im unable to start the repo. Could you fix the PR?

@nerdyman
Copy link
Owner

nerdyman commented Apr 6, 2024

@zoltan-mihalyi Ah that's bug in the Storybook test runner, I just had to add a workaround in the CI for it too. If you merge the latest main into your PR you should get the fix so the CI will run correctly.

Locally, this should fix things:

pnpm dlx [email protected] install --with-deps chromium

For the rm -rf stuff I'd recommend using WSL or Git Bash. WSL would be the better option if you already have it set up but Git Bash will allow you to keep your existing setup on Windows and give you the normal Unix/Linux commands. I don't use Windows myself so didn't really consider running the app through cmd or PowerShell.

I've updated the contrib docs to recommend using WSL on Windows.

The react-compares-slider not found issue was probably because the lib hadn't been built before Storybook started.

This flow should work:

# In one terminal run this:
pnpm --filter "react-compare-slider" dev
# In a separate terminal run this:
pnpm --filter "@this/storybook" dev

I'm looking at adding a single dev script in the root to do all this automatically.

@zoltan-mihalyi
Copy link
Contributor Author

@nerdyman I'm sorry, I ran into many problems using wsl:

  • playwright wasn't able to be installed to suse linux (I switched to ubuntu)
  • got EPERM: operation not permitted, futime error
  • received /sbin/ldconfig.real: Can't link /usr/lib/wsl/lib/libnvoptix_loader.so.1 to libnvoptix.so.1

I'm sorry but It seems I'm not able to finish this PR myself. Thanks for your understanding.

@nerdyman
Copy link
Owner

nerdyman commented Apr 7, 2024

@zoltan-mihalyi no worries, thanks for your work on this. The code looks good to me so I'll do some testing then we should be able to get this merged. I'll set up a VM to try to get the repo working on Windows too and update the contrib guide with what I find.

@zoltan-mihalyi
Copy link
Contributor Author

@nerdyman Thanks a lot!

Thinking through this feature: I think there should be a three-way switch instead of a boolean (clipItemOne) or instead of two booleans (clipItemOne and clipItemTwo). Because it is invalid to disable clipping for both. For example: clip?: 'itemOne' | 'itemTwo' | 'both' // default is 'itemTwo'.

@nerdyman
Copy link
Owner

nerdyman commented Apr 8, 2024

@zoltan-mihalyi yeah cool that makes sense to me. I'll get this reviewed and tweak the components so it can do any of the three.

@nerdyman nerdyman changed the base branch from main to next April 9, 2024 16:39
@nerdyman nerdyman merged commit 81d1fd9 into nerdyman:next Apr 11, 2024
5 of 7 checks passed
@nerdyman
Copy link
Owner

@zoltan-mihalyi I've merged this into the next branch so I'll be able to pick up the three-way switch stuff. #136 it still open so progress can be tracked there.

Thanks for your work on this!

@zoltan-mihalyi zoltan-mihalyi mentioned this pull request May 16, 2024
4 tasks
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.

Add support for clipping imageOne
2 participants