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

Individual tagpage: Add dropdown to distinct btw info and add test #5925

Merged
merged 1 commit into from Jul 5, 2019

Conversation

@CleverFool77
Copy link
Member

commented Jun 20, 2019

Fixes #5890 part
The position of dropdown button looks floated to rightmost.
The position will get fixed with the help of one #5902 where the grid system is being applied.

Screenshot from 2019-06-20 15-01-22

Screenshot from 2019-06-20 15-00-39

@plotsbot

This comment has been minimized.

Copy link
Collaborator

commented Jun 20, 2019

1 Warning
⚠️ There was an error with Danger bot’s Junit parsing: No JUnit file was found at output.xml
3 Messages
📖 @CleverFool77 Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: Fixes #123.
📖 #
Screenshots 📸 (click to expand)

5925-test_viewing_question_post.png

5925-test_wiki.png

5925-test_tag_page.png

5925-test_login.png

5925-test_wiki_page_with_inline_grids.png

5925-test_questions.png

5925-test_viewing_the_dropdown_menu.png

5925-test_stats.png

5925-test_tags.png

5925-test_people.png

5925-test_front.png

5925-test_signup.png

5925-test_questions_shadow.png

5925-test_blog.png

5925-test_front_page_with_navbar_search_autocomplete.png

5925-test_viewing_the_dashboard.png

Learn about automated screenshots

Generated by 🚫 Danger

@CleverFool77 CleverFool77 force-pushed the CleverFool77:dropp branch 2 times, most recently from 2489c49 to d3e6f2e Jun 20, 2019

@gautamig54

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

Looking great!
@CleverFool77 How do I generate related tags for a particular tag in my local system?

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

Do you still need help gautami?

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

Hi @jywarren just pushed one the parts of individual tagoage issue here.

@CleverFool77 CleverFool77 force-pushed the CleverFool77:dropp branch from d3e6f2e to e7c52bd Jun 21, 2019

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

Hi @jywarren I wanted to ask that

  test 'should have active question tab for question' do
    tag = tags(:question)

    get :show, params: { id: tag.name }
    selector = css_select "a[href = '/questions/tag/question:spectrometer']"
    assert_equal selector.size, 1
    selector = css_select '#questions.active'
    assert_equal selector.size, 1
  end

Here We are doing testing by using question.active in tab. But in new design we have dropdown. And unless dropdown menu is being shown, despite of being at questions page, that class won't be active for dropdown-item leading to failure of test. So should I add condition where unless dropdown-menu is having show class, then only the test for question dropdown-item should have active class should be tested ?

@CleverFool77 CleverFool77 force-pushed the CleverFool77:dropp branch from e7c52bd to dadce2c Jun 21, 2019

@CleverFool77 CleverFool77 changed the title [WIP] Individual tagpage: Add dropdown to distinct btw info Individual tagpage: Add dropdown to distinct btw info Jun 21, 2019

@CleverFool77 CleverFool77 requested a review from jywarren Jun 21, 2019

@CleverFool77 CleverFool77 added this to In progress PRs in UI improvements - Summer Of Code 2019 via automation Jun 21, 2019

@CleverFool77 CleverFool77 force-pushed the CleverFool77:dropp branch from dadce2c to bd1a7af Jun 21, 2019

@gautamig54

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

@CleverFool77 Do you think it's really required? Like, initially(by default) some tab say, wiki will be active and it has nothing to do with the dropdown. In that case, it might fail tests. You then have to add wikis to the tests separately.
What do you think?

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

But here it's not getting the drop down item as active at all. Because the drop down menu is closed. I provided toggle for drop down, so it closes as we chose one of the items. So i thought maybe we need to check whether dropdown menu is open, and then only check the active class for item.

Or should I remove the toggle for drop down menu . So the menu doesnt get closed itself. Then I can direct check the active class for drop down item

@gautamig54

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

I am not sure if we need to do this. I am not good with writing tests so lets wait for @jywarren's view on this one.

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Here We are doing testing by using question.active in tab. But in new design we have dropdown. And unless dropdown menu is being shown, despite of being at questions page, that class won't be active for dropdown-item leading to failure of test. So should I add condition where unless dropdown-menu is having show class, then only the test for question dropdown-item should have active class should be tested ?

Hmm, so isn't the issue perhaps that there is no fixed URL for showing questions, and perhaps there should be? What if the dropdown loads the route https://publiclab.org/questions/tag/pi-camera and then we can ensure that what's being shown is questions? this also most minimally disrupts the current routing setup and tests, even if it does require a page refresh still.

Am I understanding your question correctly?

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

Here We are doing testing by using question.active in tab. But in new design we have dropdown. And unless dropdown menu is being shown, despite of being at questions page, that class won't be active for dropdown-item leading to failure of test. So should I add condition where unless dropdown-menu is having show class, then only the test for question dropdown-item should have active class should be tested ?

Hmm, so isn't the issue perhaps that there is no fixed URL for showing questions, and perhaps there should be? What if the dropdown loads the route https://publiclab.org/questions/tag/pi-camera and then we can ensure that what's being shown is questions? this also most minimally disrupts the current routing setup and tests, even if it does require a page refresh still.

Am I understanding your question correctly?

Hi @jywarren I didn't understand the point clearly though.
But I thought of a method. I'll remove the test written for tab and write new tests for dropdown directly.

@CleverFool77 CleverFool77 force-pushed the CleverFool77:dropp branch from bd1a7af to df496db Jun 24, 2019

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2019

HI Jeff,
I was going through the tests and I need some help.
I'm not unable to understand how to check something which is conditional.
As in this, the tab earlier were direct, and so was the test. But now in dropdown, The dropdown-menu dissappears and toggles to button after clicking on one of the items. So If I have to test the item, its necessary to have the dropdown-menu to be shown.
I tried some methods like checking dropdown menu has show class and etc. But they doesn't seem to work. 🤔

<li class="nav-item"><a class="nav-link <% if @node_type == "contributors" %> active <% end %>" href="/contributors/<%= params[:id] %>"><i class="fa fa-user"></i> <span class="d-none d-lg-inline"><%= raw t('tag.show.contributors') %></span><span class="badge"><%=@length %></span></a></li>
<div class="dropdown-menu dropdown-menu-right mt-1" aria-labelledby="dropdownMenuButton">
<% if params[:action] == "show" %>
<% unless params[:id].match("question:") %><a class="dropdown-item <% if @node_type == "note" %> active<% end %>" href="/tag/<%= params[:id] %>"><i class="fa fa-file"></i> <span class="d-none d-lg-inline"><%= raw t('tag.show.research_notes') %></span></a><% end %>

This comment has been minimized.

Copy link
@jywarren

jywarren Jun 26, 2019

Contributor

Can u use the dev console elements inspector to see what the code looks like? I mean, here, it seems like we should be able to test against it with css_select. Maybe css_select docs say that the element has to be shown?

If you can't get it to work, you could write a system test for it, where you literally send a click event to open the dropdown. I've done that for some system tests, so it's a backup plan!

This comment has been minimized.

Copy link
@CleverFool77

CleverFool77 Jun 26, 2019

Author Member

But Doesn't css_select work only when the element is being shown not when it's hidden like dropdown menu here ?
I did system test though, to have a screenshot when menu is open and closed.

@CleverFool77 CleverFool77 force-pushed the CleverFool77:dropp branch 3 times, most recently from f652ba7 to 25d8335 Jun 26, 2019

@CleverFool77 CleverFool77 changed the title Individual tagpage: Add dropdown to distinct btw info Individual tagpage: Add dropdown to distinct btw info and add test Jun 26, 2019

@CleverFool77 CleverFool77 force-pushed the CleverFool77:dropp branch from 25d8335 to 1db93b2 Jun 26, 2019

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

 FAIL["test_viewing_the_dashboard", #<Minitest::Reporters::Suite:0x00005653ac855f38 @name="DashboardTest">, 129.53698157000002]
 test_viewing_the_dashboard#DashboardTest (129.54s)
        expected to find visible css ".row.header > h1" with text "Dashboard" but there were no matches. Also found "Community research", which matched the selector but not all filters. 
        test/system/dashboard_test.rb:19:in `block in <class:DashboardTest>'

Uhmm !! 🤔 This test seems to failing despite there being no change related to this. The reason for this is that there is no matches for text Dashboard text 🤔

@CleverFool77 CleverFool77 force-pushed the CleverFool77:dropp branch from 1db93b2 to 63c7b4e Jun 26, 2019

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

All checks passed 🎉 🎉

@jywarren
Copy link
Contributor

left a comment

This looks super. Can we wait a little bit so we can merge more of these changes at one time? Thanks!

class TagTest < ApplicationSystemTestCase
Capybara.default_max_wait_time = 60

test 'viewing the dropdown menu' do

This comment has been minimized.

Copy link
@jywarren

jywarren Jun 27, 2019

Contributor

Very awesome!

@jywarren jywarren added the ready label Jun 27, 2019

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

Sure Jeff !!!
Thanks !!

@CleverFool77 CleverFool77 referenced this pull request Jul 3, 2019
4 of 5 tasks complete

@jywarren jywarren merged commit 19aa1c2 into publiclab:master Jul 5, 2019

3 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
danger/danger ⚠️ 1 Warning. Don't worry, everything is fixable.
Details

UI improvements - Summer Of Code 2019 automation moved this from In progress PRs to Done Jul 5, 2019

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

OK, merged #5948 and this. Now to #5938 !!!

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

Cool !!

enviro3 added a commit to enviro3/plots2 that referenced this pull request Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.