Skip to content

[Enhancement]: [Search] Scrollable search result #1350

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 14, 2020
Merged

[Enhancement]: [Search] Scrollable search result #1350

merged 2 commits into from
May 14, 2020

Conversation

patilkiranm
Copy link
Contributor

@patilkiranm patilkiranm commented Mar 2, 2020 •
edited by lubber-de
Loading

Description

Now it is possible to make search results scroll-able.

  1. Handles keyboard events
  2. Regains scroll position after closing of results due to blur/click event

Screenshot

Search: Without category
ScrollableSearch

Search: Withcategory
ScrollableSearch2

Closes

Semantic-Org/Semantic-UI#6488 via new long and short classes

@patilkiranm
[Enhancement]: Search widget, Scrollable search result.
fb5e9e7 
1. Handles keyboard events
2. Regains scroll position after closing of results due to blur/select event
@lubber-de lubber-de added lang/javascript Anything involving JavaScript state/awaiting-reviews Pull requests which are waiting for reviews type/feat Any feature requests or improvements labels Mar 2, 2020
lubber-de
lubber-de requested changes Mar 3, 2020
View reviewed changes
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.

Thanks for this PR.
You need to mention that this is only working when the .results class has an overflow and fixed height set.
So i suggest you also implement additional classes similiar to the ones we have for dropdown already,so your change is actually of any use (out of the box)

.ui.scrolling.search > .results
.ui.short.scrolling.search > .results
.ui.very.short.scrolling.search > .results
.ui.long.scrolling.search > .results
.ui.very.long.scrolling.search > .results

Please make sure the height settings are defined as variables and support responsive widths via media queries just like we have for dropdown

Fomantic-UI/src/definitions/modules/dropdown.less

Lines 461 to 471 in 62bd670

& when (@variationDropdownLong) {
.ui.selection.dropdown.long .menu {
max-height: @selectionMobileMaxMenuHeight * 2;
}
.ui.selection.dropdown[class*="very long"] .menu {
max-height: @selectionMobileMaxMenuHeight * 3;
}
}
}
@media only screen and (min-width: @tabletBreakpoint) {
& when (@variationDropdownShort) {

When done it would be very nice if you could also create a PR for a proper docs example 😉

Examples

Current behavior

Applied your PR

Adjusted CSS

@lubber-de lubber-de added the state/awaiting-docs Pull requests which need doc changes/additions label Mar 3, 2020
@lubber-de lubber-de added this to the 2.8.x milestone Mar 3, 2020
@exoego
Copy link
Contributor

exoego commented Mar 3, 2020 •
edited
Loading

I tested this PR in http://jsfiddle.net/gtk760c3/2/ added by @lubber-de .
Scrolling is useful if search results contains tens of entries, but I think the height of scrollbale result in current implementation is a bit shorter: it can only show 2-3 lines.
image

@patilkiranm
[Enhancement]: Search widget, Scrollable search result.
4978250 
1. Handles keyboard events
2. Regains scroll position after closing of results due to blur/select event
3. Variation classes added such as scrolling, short, long, very short and very long
@patilkiranm
Copy link
Contributor Author

@lubber-de Scrolling and height variations added to results :)

lubber-de
lubber-de approved these changes Mar 5, 2020
View reviewed changes
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.

LGTM
I prepared a testcase where the additional classes are implemented here
http://jsfiddle.net/yj4nhqkc/

@lubber-de lubber-de modified the milestones: 2.8.x, 2.8.5 Mar 5, 2020
exoego
exoego approved these changes Mar 6, 2020
View reviewed changes
Copy link
Contributor

@exoego exoego left a comment

Choose a reason for hiding this comment

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

LGTM 🥇

@patilkiranm patilkiranm mentioned this pull request Mar 6, 2020
Merged
@lubber-de
Copy link
Member

@patilkiranm Please see my comment from the docs PR fomantic/Fomantic-UI-Docs#199 (review), because the scrolling only has effect on mobile/small devices (or if the maxResults value is drastically increased

@lubber-de lubber-de added state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo and removed state/awaiting-docs Pull requests which need doc changes/additions labels Mar 6, 2020
ColinFrick
ColinFrick approved these changes Mar 8, 2020
View reviewed changes
Copy link
Member

@ColinFrick ColinFrick left a comment

Choose a reason for hiding this comment

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

LGTM

@lubber-de lubber-de merged commit 7cca9f3 into fomantic:develop May 14, 2020
@lubber-de lubber-de removed the state/awaiting-reviews Pull requests which are waiting for reviews label May 18, 2020
@lubber-de lubber-de mentioned this pull request Jun 1, 2020
Closed
@lubber-de
Copy link
Member

@all-contributors please add @patilkiranm for code and doc

@allcontributors allcontributors bot mentioned this pull request Aug 16, 2021
Merged
@allcontributors
Copy link
Contributor

@lubber-de

I've put up a pull request to add @patilkiranm! 🎉

@dreaming-augustin dreaming-augustin mentioned this pull request Dec 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@exoego exoego exoego approved these changes

@ColinFrick ColinFrick ColinFrick approved these changes

@lubber-de lubber-de lubber-de approved these changes

@y0hami y0hami Awaiting requested review from y0hami

@prudho prudho Awaiting requested review from prudho

Assignees
No one assigned
Labels
lang/javascript Anything involving JavaScript state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo type/feat Any feature requests or improvements
Projects
None yet
Milestone
2.8.5
Development

Successfully merging this pull request may close these issues.
4 participants
@patilkiranm @exoego @lubber-de @ColinFrick