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

[Search] trigger 'focusin', 'focusout'. #3144

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

dreaming-augustin
Copy link
Contributor

@dreaming-augustin dreaming-augustin commented Dec 24, 2024

Trigger 'focusin' and 'focusout' whenever a search result item becomes active.

It allows websites to implement a solution for this:

[Search] Bring active result into view #3143

It allows websites to implement a solution for this:

[Search] Bring active result into view fomantic#3143
fomantic#3143
@dreaming-augustin
Copy link
Contributor Author

This patch at least allows for a partial solution to [Search] Bring active result into view #3143

It allows websites to implement their own solution, according to their needs.
A website in now able to implement something like the following on their own:

        // Scroll  search results into view.
        $('#search').on('focusin', '.result', function() {
                var offset = $(this).offset();
                var viewportTop    = $(window).scrollTop();
                var viewportHeight = $(window).height();
                var viewportBottom = viewportTop + viewportHeight;
                var elementTop     = $(this).offset().top;
                if (elementTop > (viewportBottom - 150)) {
                        offset.top = elementTop - (viewportHeight / 4);
                        $('html, body').animate({
                                scrollTop: offset.top,
                        }, 200);
                }
                if (elementTop < (viewportTop + 150)) {
                        offset.top = elementTop - (3 * viewportHeight / 4);
                        $('html, body').animate({
                                scrollTop: offset.top,
                        }, 200);
                }
        });

This feels a bit hackish and too specific. It can be discussed whether something more appropriate could be considered to be included within Fomantic.

At the very least, triggering focusin(), developers have more options to deal with their own business needs.

Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

While this change does not seem to hurt the code (check my comment if focusout is really needed if we add focusin at the same time),
i wonder if this is really needed. The user could already make use of the mutationobserver and check for the added ".active" class

@@ -329,8 +329,10 @@
;
$result
.removeClass(className.active)
.trigger('focusout')
Copy link
Member

@lubber-de lubber-de Dec 25, 2024

Choose a reason for hiding this comment

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

Do we really need this? The direct focusin trigger 3 lines below should automatically trigger the focusout on the previous focused element, isnt it?

@lubber-de
Copy link
Member

lubber-de commented Dec 25, 2024

Btw we should not implement automatic scrolling of the core page into the FUI Code. I believe this will mess up existing projects in unexpected scenarios

@lubber-de lubber-de added type/feat Any feature requests or improvements state/awaiting-reviews Pull requests which are waiting for reviews javascript Pull requests that update Javascript code labels Dec 25, 2024
@dreaming-augustin
Copy link
Contributor Author

Btw we should not implement automatic scrolling of the core page into the FUI Code. I believe this will mess up existing projects in unexpected scenarios

Of course not. I said so myself. The code I shared within a comment is a hack that works well in a very specific scenario where I needed it. I shared it so that interested Fomantic users can adapt it to their needs. But it is too specific to be included in Fomantic itself.

... Now, if the community can come up with something more general and acceptable for Fomantic, I'd be happy to test it out...

@dreaming-augustin
Copy link
Contributor Author

i wonder if this is really needed. The user could already make use of the mutationobserver and check for the added ".active" class

I will investigate and come back to you soon.

@dreaming-augustin
Copy link
Contributor Author

I didn't know the existence of MutationObservers...
I will learn:
https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code state/awaiting-reviews Pull requests which are waiting for reviews type/feat Any feature requests or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants