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

Fix #965 - Reset button/item count overlaps search input query on fil… #1007

Merged
merged 4 commits into from
Aug 12, 2021

Conversation

maxxcrawford
Copy link
Contributor

@maxxcrawford maxxcrawford commented Aug 11, 2021

This PR fixes:

  • Issue where the reset button/item count overlaps search input query on filter bar
  • Use case where user refreshes page while items are being filtered
    • Including showing the search bar on mobile
  • Use case where a user has filtered aliases and is not focused on the search query input but wants to reset
  • Protocol / JS optimizations
  • Issue where page throws JS error bc element is not visible

STR:

  • Go to Relay dashboard
  • Enter long string (longer than the width of the input box) into the search filter bar
  • Expected: The input should "stop" before the X/X number of items visible/total.

<span class="js-filter-case-visible"></span>
/
<span class="js-filter-case-total"></span>
<div class="c-filter-search-controls">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapped the .c-filter-case-count and .c-filter-reset in a containing div.

Comment on lines +535 to +536
if ( document.getElementById("survey-dismiss") ) {
document.getElementById("survey-dismiss").addEventListener("click", dismissSurvey, false);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was throwing a JS error on pages where the micro-survey-banner did not exist.

Comment on lines +30 to +34
let query = input;

if (input.target) {
query = input.target.value.toLowerCase();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

let reasoning: this function was retro fit to run on page load if there was a value in the search filed. (Use case: user refreshes the page while some aliases are being filtered)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxxcrawford maxxcrawford marked this pull request as ready for review August 11, 2021 19:17
@maxxcrawford maxxcrawford requested a review from codemist August 11, 2021 19:17
…e load, add logic to keep the reset button visible if the search bar contains a query, revise JS styles to toggle visibility classes instead of manipulating style.display,
@maxxcrawford maxxcrawford force-pushed the 965-search-bar-overlap branch from bb0717d to 25fc7f0 Compare August 12, 2021 12:33
Copy link
Collaborator

@codemist codemist left a comment

Choose a reason for hiding this comment

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

Everything looks good!

@maxxcrawford maxxcrawford merged commit fc3940a into main Aug 12, 2021
@maxxcrawford maxxcrawford deleted the 965-search-bar-overlap branch August 12, 2021 15:48
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.

2 participants