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

modify power tag display to use mini cards #8684

Closed
wants to merge 10 commits into from
Closed

Conversation

jywarren
Copy link
Member

@jywarren jywarren commented Oct 27, 2020

Builds on part of #6593 --

Using https://getbootstrap.com/docs/4.4/components/card/

image

After, we are aiming for the badges to look like this:

image

@gitpod-io
Copy link

gitpod-io bot commented Oct 27, 2020

@codecov
Copy link

codecov bot commented Oct 27, 2020

Codecov Report

Merging #8684 into main will increase coverage by 0.23%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8684      +/-   ##
==========================================
+ Coverage   81.77%   82.01%   +0.23%     
==========================================
  Files         101      100       -1     
  Lines        5905     5889      -16     
==========================================
+ Hits         4829     4830       +1     
+ Misses       1076     1059      -17     
Impacted Files Coverage Δ
app/controllers/tag_controller.rb 81.45% <100.00%> (ø)
app/models/node.rb 91.45% <100.00%> (ø)
app/models/node_tag.rb 95.23% <0.00%> (-4.77%) ⬇️
app/api/srch/search.rb 70.70% <0.00%> (+3.82%) ⬆️
app/services/execute_search.rb 94.44% <0.00%> (+5.55%) ⬆️

@jywarren
Copy link
Member Author

Needs a bit more refinement! But getting there:

image

With more spacing maybe we are good for an initial pass.

@jywarren
Copy link
Member Author

ok, removed followers count from power tags

@jywarren
Copy link
Member Author

jywarren commented Oct 27, 2020

 test_removing_tags_from_the_post#PostTest (179.08s)
Minitest::UnexpectedError:         Capybara::ExpectationNotMet: expected to find visible css ".tags-list .card-body" 2 times, found 3 matches: "awesome\n3 following\nFollow", "chapter\n1 following\nFollow", "everything"
            test/system/post_test.rb:52:in `block in <class:PostTest>'

expected to find visible css ".tags-list .card-body" 2 times, found 3 matches

@jywarren
Copy link
Member Author

jywarren commented Nov 5, 2020

OK - errors:

 FAIL["test_adding_a_location_to_the_wiki", #<Minitest::Reporters::Suite:0x00007fcbcfa1e738 @name="PostTest">, 239.69259713200003]
 test_adding_a_location_to_the_wiki#PostTest (239.69s)
        expected to find css ".tags-list .badge a[href=\"/tag/lat:22\"]" but there were no matches
        test/system/post_test.rb:210:in `block in <class:PostTest>'


=======[Screenshot]: tmp/screenshots/failures_test_removing_tags_from_the_post.png
ERROR["test_removing_tags_from_the_post", #<Minitest::Reporters::Suite:0x00007fcbcee37708 @name="PostTest">, 329.98693319999995]
 test_removing_tags_from_the_post#PostTest (329.99s)
Minitest::UnexpectedError:         Capybara::ElementNotFound: Unable to find css ".tags-list p.badge .tag-delete"
            test/system/post_test.rb:57:in `block (2 levels) in <class:PostTest>'
            test/system/post_test.rb:56:in `block in <class:PostTest>'

@@ -1,4 +1,4 @@
<div class="card" style="clear:left;">
<div class="card<% if local_assigns[:hide_button] %> more-tags<% end %>" style="clear:left;<% if local_assigns[:hide_buttong] %>display:none;<% end %><% if local_assigns[:grey] %>background:#eee;<% end %>">
Copy link
Member Author

Choose a reason for hiding this comment

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

we could redo the background here using css and then system test for it...

@jywarren
Copy link
Member Author

jywarren commented Nov 5, 2020

OK cool - so on adding tags, they don't yet display in cards, so we are seeing this first error:


 test_adding_tags_to_the_post#PostTest (166.06s)
Minitest::UnexpectedError:         Capybara::ExpectationNotMet: expected to find css ".tags-list .card" 2 times but there were no matches
            test/system/post_test.rb:43:in `block in <class:PostTest>'

Then, probably we have to update the selectors to the card-style deletion buttons: (DONE)

==[1105/201552.271467:WARNING:spdy_session.cc(3384)] Received HEADERS for invalid stream 121
====[Screenshot]: tmp/screenshots/failures_test_removing_tags_from_the_post.png
ERROR["test_removing_tags_from_the_post", #<Minitest::Reporters::Suite:0x00007f4abc139a30 @name="PostTest">, 254.78303012099997]
 test_removing_tags_from_the_post#PostTest (254.78s)
Minitest::UnexpectedError:         Capybara::ExpectationNotMet: expected to find visible css ".tags-list .card" 0 times, found 3 matches: "awesome\n3 following\nFollow", "chapter\n1 following\nFollow", "everything"
            test/system/post_test.rb:54:in `block in <class:PostTest>'

@codeclimate
Copy link

codeclimate bot commented Nov 5, 2020

Code Climate has analyzed commit b007b81 and detected 0 issues on this pull request.

View more on Code Climate.

@jywarren
Copy link
Member Author

jywarren commented Nov 5, 2020

Great, next we'll get the tag display for JS/Ajax inserted tags to be a card, not a badge:

function displayNewTag(tag_name, tag_id, deletion_path) {
$('.tags-list:first').append("<p id='tag_"+tag_id+"' class='badge badge-primary m-0'> \
<a class='tag-name' style='color:white;' href='/tag/"+tag_name+"'>"+tag_name+"</a> <a class='tag-delete' \
data-remote='true' href='"+deletion_path+"/"+tag_id+"' data-tag-id='"+tag_id+"' \
data-method='delete'><i class='fa fa-times-circle fa-white blue pl-1' aria-hidden='true' ></i></a></p> ")
setupTagDelete($('#tag_' + tag_id + ' .tag-delete'));
}

Could we load it as a partial, from Rails, maybe, so it's identical, and not duplicated code? It'd take a new controller action, i think.

@ebarry
Copy link
Member

ebarry commented Feb 23, 2021

excited for this to merge!

@stale
Copy link

stale bot commented Feb 24, 2022

Hi 😄, this issue has been automatically marked as stale because it has not had recent activity. Don't worry you can continue to work on this and ask @publiclab/reviewers to add in-progress label 🎉 . Otherwise, it will be closed if no further activity occurs in 10 days -- but you can always re-open it if you like! 💯 Thank you for your contributions! 🙌 🎈.

@stale stale bot added the stale label Feb 24, 2022
@stale stale bot closed this Mar 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants