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 optional text expansion on mouseover #1

Merged
merged 10 commits into from Feb 25, 2022
Merged

Conversation

jakeisnt
Copy link
Contributor

This PR introduces a boolean flag that can be passed to createTelescopicTextFromBulletedList, false by default, that turns on the ability to expand text when the text is moused over. Notably, the text is not expanded recursively when moused over; we use a very short time delay to ensure this doesn't happen

(if there is a better way to do this, let me know! this might be dependent on browser performance)

Usage is fairly self-explanatory, but please let me know if and how you'd like it to be documented! Happy to add a demo as well if you'd like - please try it out!

@spencerc99
Copy link
Collaborator

oh fun! can you add a video showing the behavior?

Also I think this behavior might make more sense if it un-expands after your mouse stops hovering? that way you can click to intentionally open rather than accidentally opening a bunch and having no way to revert. You'll probably need to add another class just to mark that it was opened from hover and not actually clicked open in order to handle not unexpanding things that were explicitly clicked open.

thanks for the contribution and idea :)

@jakeisnt
Copy link
Contributor Author

I like the hover idea - and I can add it - but I think that's a separate feature!

I tried out this initial expansion when hovering, then "locking in" the expansion by clicking, but it didn't feel intuitive; something impactful should change on click, and expanding it twice doesn't 'feel right' to me. I like being able to move the mouse around to interact with the page without clicking at all - that was the motivation here.

@jakeisnt
Copy link
Contributor Author

Somewhat unrelated, but I think that passing a toplevel 'config' object argument rather than having to deal with more than one optional parameter would be a better move here - that way users aren't required to provide all of the optional parameters prior to the one they want to use.

@spencerc99
Copy link
Collaborator

Somewhat unrelated, but I think that passing a toplevel 'config' object argument rather than having to deal with more than one optional parameter would be a better move here - that way users aren't required to provide all of the optional parameters prior to the one they want to use.

ah yes that would be great, want to bundle that in here and make an Config interface that has separator and hoverable?

I like the hover idea - and I can add it - but I think that's a separate feature!

I tried out this initial expansion when hovering, then "locking in" the expansion by clicking, but it didn't feel intuitive; something impactful should change on click, and expanding it twice doesn't 'feel right' to me. I like being able to move the mouse around to interact with the page without clicking at all - that was the motivation here.

mmm interesting ok that's fair! interested in seeing the interaction on video!

@jakeisnt
Copy link
Contributor Author

Quick video - took me a sec, had to get screen recording up on this linux system...
https://user-images.githubusercontent.com/29869612/155260730-64500871-8932-4dbc-ae8f-e07e4d55f63b.mp4

Copy link
Collaborator

@spencerc99 spencerc99 left a comment

Choose a reason for hiding this comment

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

overall looking good to me! just some final things

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
jakeisnt and others added 2 commits February 23, 2022 08:18
Co-authored-by: Spencer Chang <spencerc99@gmail.com>
Co-authored-by: Spencer Chang <spencerc99@gmail.com>
Copy link
Owner

@jackyzha0 jackyzha0 left a comment

Choose a reason for hiding this comment

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

tentative approval from me, will wait until @spencerc99 looks again before we merge! this is super cool

Copy link
Collaborator

@spencerc99 spencerc99 left a comment

Choose a reason for hiding this comment

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

some small comments otherwise looks great!! thanks for the suggestion and change :)

src/index.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@jakeisnt
Copy link
Contributor Author

Should be ready to go! Sorry about the silly mistakes

@jackyzha0 jackyzha0 merged commit d8480cb into jackyzha0:main Feb 25, 2022
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.

None yet

3 participants