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

add support for resizing elements #569

Merged
merged 6 commits into from
Oct 4, 2022
Merged

add support for resizing elements #569

merged 6 commits into from
Oct 4, 2022

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented Oct 2, 2022

Closes #17

  • Changed the svg circles in visbug-handles to the new visbug-handle element.
  • Pluralized existing variables for handle-css and HandleStyles and added new singular ones for the element
  • The new element takes an attribute called placement. e.g. <visbug-handle placement="top-end">.
  • This attribute handles both the conditional styling as well as the conditional logic for resizing.

Order of events is as follows:

  1. pointerdown on visbug-handle triggers on_element_resize_start
    • this saves the initial state of the resizing
    • adds listeners for pointermove and pointerup on document
  2. pointermove on document triggers on_element_resize_move
    • this is where the resizing is calculated and triggered via requestAnimationFrame
  3. pointerup on document triggers on_element_resize_end
    • this is where we detach the pointermove listener
    • also restore any original styles that we modified for the resize (e.g. cursor and transition)

Screen recording showing a demo of resizing various elements

@google-cla
Copy link

google-cla bot commented Oct 2, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Member

@argyleink argyleink left a comment

Choose a reason for hiding this comment

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

overall i saw no issues with the new feature 🤘🏻
the local dev website rocks, and it performed super well on all sorts of live websites.
it was super fun! this should land awesomely!

there's a couple little suggestions in review comments, but nothing major.

i did notice some transform drift when resizing from the top? i think it's acceptable given the illusion working so well. the handles don't drift is what i mean, and i think that was a good choice and this is an ok side effect.

any closing thoughts for you before this merges and you contribute a rad feature?

Comment on lines 5 to 7
grid-area: 1 / -1;
place-self: var(--align-self, center) var(--justify-self, center);
transform: translate(var(--translate-x, 0), var(--translate-y, 0));
Copy link
Member

Choose a reason for hiding this comment

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

love this ❤️

Comment on lines 16 to 21
background-color: white;
border: 1px solid hotpink;
padding: 0;
width: 0.5rem;
height: 0.5rem;
border-radius: 50%;
Copy link
Member

Choose a reason for hiding this comment

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

thx for stickin close to the original style while updating it to something interactive 👍🏻

aka, nice intuitive design decisions here

position: relative;
cursor: var(--cursor);

/* increase tap target size */
Copy link
Member

Choose a reason for hiding this comment

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

i noticed this UX and went looking for what you did! rad inset style yo

}
}

:host([placement^="top"]) {
Copy link
Member

Choose a reason for hiding this comment

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


connectedCallback() {
this.$shadow.adoptedStyleSheets = this.styles
this.$shadow.innerHTML = this.render();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.$shadow.innerHTML = this.render();
this.$shadow.innerHTML = this.render()

😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b1c9084. I also noticed that the project uses spaces everywhere, whereas my IDE default is tabs so fixed that too.

Might help to set up prettier

Copy link
Member

Choose a reason for hiding this comment

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

theres an editor config, but not everyone has that plugin installed

this.$shadow.innerHTML = this.render();

this.button = this.$shadow.querySelector('button')
this.button.addEventListener('pointerdown', this.on_element_resize_start.bind(this))
Copy link
Member

Choose a reason for hiding this comment

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

i bet this'll work with touch because of this choice 👍🏻
should be fine to try!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i dont have a touch device (that supports browser extensions) to try this but yeah that's the idea behind using pointerdown vs mousedown

i think there are additional optimizations that can be made in touch events that are not available in pointer events. one example is the touches to prevent unexpected behavior when using gestures.

but the code is much simpler with pointerdown 🤷

requestAnimationFrame(() => {
sourceEl.style.width = `${newWidth}px`
sourceEl.style.height = `${newHeight}px`
sourceEl.style.transform = `translate(${newTranslate.x}px, ${newTranslate.y}px)`
Copy link
Member

Choose a reason for hiding this comment

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

very clever to transform here! awesome UX attention to detail here, really sells the illusion. the same technique would probably support margin and padding visuals being grab-able 🤔

also cool you used the DOMMatrix to do the math 🤘🏻

not two things i would have reached for immediately, but find they totally rock for this scenario. cool math and programming plus a great user experience. 😍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jake deserves the credit for DOMMatrix 😄 https://youtu.be/VdNzD4lhidw

Copy link
Member

Choose a reason for hiding this comment

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

i was wonderin!!! seemed like appropriate timing

app/components/selection/handle.element.js Outdated Show resolved Hide resolved
@argyleink argyleink merged commit 317ab7d into GoogleChromeLabs:main Oct 4, 2022
@argyleink
Copy link
Member

everyone will get the update within the hour!
thank you for this, it's raddddd!

@mayank99
Copy link
Contributor Author

mayank99 commented Oct 4, 2022

thanks for the lovely comments and for accepting the contribution 💜

the process was mostly smooth, but i couldn't get dev:extension to work on windows (it uses cp) so i only tested in the local dev environment

i did notice some transform drift when resizing from the top? i think it's acceptable given the illusion working so well. the handles don't drift is what i mean, and i think that was a good choice and this is an ok side effect.

yeah... i noticed that this issue happens only on some elements but not others. i could not figure out why, it might have something to do with the layout?

@mayank99 mayank99 deleted the mayank/resize branch October 4, 2022 15:04
@argyleink
Copy link
Member

i couldn't get dev:extension to work on windows (it uses cp) so i only tested in the local dev environment

i was wondering if you'd tried loading the extension unpacked..! i'm sad that cp prevented that, sorry about that! there's certainly some infra updates that could be done in the codebase to smooth out cross platform and cross IDE dev. need to block out some maintenance time. also need to update to manifest v3. thanks for letting me know tho! want to log a bug about it?

p.s.
looks like auto publishing github action stopped working, just manually uploaded

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.

Resize
2 participants