Skip to content

DEV: Refactor tests & add animation #32547

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

Merged
merged 13 commits into from
May 15, 2025
Merged

DEV: Refactor tests & add animation #32547

merged 13 commits into from
May 15, 2025

Conversation

jordanvidrine
Copy link
Contributor

@jordanvidrine jordanvidrine commented May 1, 2025

This PR:

  • only injects component into header when it should be shown, rather than relying on CSS to hide it
  • corrects erroneous positives from tests & migrates test to Rspec
  • eliminates a super quick rendering / disappearing of the search header on page load when the welcome banner search is present
  • adds an animation to the search in header when it appears on scrolling past the welcome banner

New

new-scroll-headersearch.mp4

Old

old-scroll-headersearch.mp4

@jordanvidrine jordanvidrine marked this pull request as ready for review May 2, 2025 20:47
@jordanvidrine jordanvidrine changed the title UX: Slight refactor to use animations DEV: Refactor tests & add animation May 2, 2025

RSpec.describe "Header Search - Responsive Behavior", type: :system do
fab!(:user)
fab!(:topics) { Fabricate.times(20, :post).map(&:topic) }
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to do this, it's slow and we have a "cheat" way of doing this type of thing where you need to scroll a lot vertically:

def fake_scroll_down_long(selector_to_make_tall = "#main-outlet")
# Trick to give a huge vertical space to scroll
page.execute_script(
"document.querySelector('#{selector_to_make_tall}').style.height = '10000px'",
)
sleep 0.1 # most resilient solution for now
page.scroll_to(0, 1000)
end

fake_scroll_down_long

visit "/"

# Default desktop view should show search field
expect(page).to have_no_css(".floating-search-input")
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a page object for search related system spec stuff already https://github.com/discourse/discourse/blob/abee294616a064e4de40d8e8eb052ea571e2203c/spec/system/page_objects/pages/search.rb , you should use this where possible in this spec

Copy link
Contributor

@martin-brennan martin-brennan left a comment

Choose a reason for hiding this comment

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

Approving with caveat that the system spec needs a bit of extra work

@jordanvidrine
Copy link
Contributor Author

Approving with caveat that the system spec needs a bit of extra work

@martin-brennan would you mind going through again? I added in all of your reccomendations

@jordanvidrine jordanvidrine merged commit 9cb3e2c into main May 15, 2025
16 checks passed
@jordanvidrine jordanvidrine deleted the search-refactor branch May 15, 2025 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants