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

Try and add filter search warning when no results is returned #344

Merged
merged 5 commits into from
Mar 18, 2019
Merged

Try and add filter search warning when no results is returned #344

merged 5 commits into from
Mar 18, 2019

Conversation

kakirastern
Copy link
Contributor

@kakirastern kakirastern commented Mar 5, 2019

To follow up on PR #340. To try and find a way to add back filter search warning when no tutorials are returned on a filter search.

Fixes #337.

@kakirastern
Copy link
Contributor Author

Testing if $.ajax()-related latency issues are the root cause of the 1st-time-load-tutorials bug...

@kakirastern
Copy link
Contributor Author

Yes, @MananAgarwal was right, it is indeed a latency issue. I have corrected for it by adding the option async: false to the two instances of JQuery $.ajax()in the file "searchtools.js". Think it is fixed now. Please check @MananAgarwal and @adrn. Thanks!

@kakirastern
Copy link
Contributor Author

Wait... seems like the same problem is cropping up on my other browser. Let me double check.

@kakirastern
Copy link
Contributor Author

I discovered the behavior is browser dependent too. Will need to work on it more before we meet next Tuesday and see if there is a way around it...

@kakirastern
Copy link
Contributor Author

kakirastern commented Mar 6, 2019

So for my Safari, I see this on my screen:

screenshot 2019-03-06 13 54 15

I don't get this with Chrome or Firefox, which is more like:

screenshot 2019-03-06 13 56 35

@kakirastern
Copy link
Contributor Author

I can confirm that the fix works only for Chrome, but not Firefox nor Safari... 🙁

@kakirastern
Copy link
Contributor Author

But I reckon there must be another way to do this... Will need to experiment and investigate some more, but it will take time

@kakirastern
Copy link
Contributor Author

@MananAgarwal I found this post and think it might be relevant for finding a solution to our current problem... https://stackoverflow.com/a/28862458/9959073. So a potential solution would be implement something like

if(!tbody.find('td').hasClass(settings.highlightClass)) {
    $('body').find('#empty').text('No results').show();
} else {
    $('body').find('#empty').hide();
}

together with

...
</table>
<p id="empty"></p>
</body>

Except in their case it is for jQuery.FilterTable to return an alert when no results found... I am not sure if it will work for the current code base though. What do you think?

@kakirastern
Copy link
Contributor Author

Or this one here might work: https://stackoverflow.com/a/14340948/9959073...

@kakirastern
Copy link
Contributor Author

kakirastern commented Mar 12, 2019

I just attempted another fix so that the code now works for both Google Chrome and Mozilla Firefox browsers, and the 1st-time-load error no longer bugs these two kinds of browsers. What I did was to add jQuery.ajaxSetup({async:false}); to the very beginning of the file "tutorials/themes/tutorials-theme/static/searchtools.js", to make sure that ajaxSetup is set to synchronous to make all $.get and $.ajax calls synchronous, which could be in bad form in some other context for some other JS file.

I am not sure what is going wrong with the Apple Safari browser... Maybe if we merge this code a warning about this site being optimized for both Chrome and Firefox would be necessary.

@MananAgarwal
Copy link
Contributor

Hey @kakirastern, it is working for me on chrome, so I think we are on the right track to figuring this out. I am a little preoccupied with my midterm exams. I'll look into this properly on the weekend.

@kakirastern
Copy link
Contributor Author

No worries @MananAgarwal! I have checked the the latest solution sometimes work on my Firefox browser, and sometimes it does not... Anyway, we can discuss later. All the best with your midterm exams!

@adrn
Copy link
Member

adrn commented Mar 18, 2019

Ah, great! This works for me locally too. Why do you say it doesn't work on Firefox or Safari? It seems to work for me on all browsers.

@adrn
Copy link
Member

adrn commented Mar 18, 2019

Thanks for this - given that it solves the issue for at least Chrome and Safari, I'm going to merge and we can discuss whether or not it works for all browsers over in #337

@adrn adrn merged commit 731f652 into astropy:master Mar 18, 2019
@kakirastern
Copy link
Contributor Author

Hi @adrn! It's weird. I checked the giles version via incognito mode using both Firefox and Safari and it wasn't working. Let me check again now the PR's been merged. Wait... (may take a while).

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