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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add featured question section to shadow question page #5861

Merged
merged 1 commit into from Jun 14, 2019

Conversation

@CleverFool77
Copy link
Member

commented Jun 8, 2019

Fixes #5733 part

Screenshot from 2019-06-08 22-57-13

  • PR is descriptively titled 馃搼 and links the original issue above 馃敆
  • tests pass -- look for a green checkbox 鉁旓笍 a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 馃搧
  • screenshots/GIFs are attached 馃搸 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below
@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2019

Hi @jywarren @gautamig54 Just added the featured question section.
Thanks !!

@jywarren
Copy link
Contributor

left a comment

Hi, so is this driven from a tagname for featured questions? Thanks!!!

@@ -27,6 +27,15 @@ def index
.paginate(page: params[:page], per_page: 24)
end

def index_shadow
@title = 'Questions and Answers'
set_sidebar

This comment has been minimized.

Copy link
@jywarren

jywarren Jun 8, 2019

Contributor

Is this line maybe not needed?

This comment has been minimized.

Copy link
@CleverFool77

CleverFool77 Jun 10, 2019

Author Member

Hi @jywarren Did you mean we don't need index_shadow in controller ?
Thanks !!

This comment has been minimized.

Copy link
@CleverFool77

CleverFool77 Jun 10, 2019

Author Member

But here that would be the method doing whole stuff in this PR.

This comment has been minimized.

Copy link
@jywarren

jywarren Jun 10, 2019

Contributor

just the set_sidebar part, actually, sorry for ambiguity!

@CleverFool77 CleverFool77 force-pushed the CleverFool77:featured-question-section branch from 9d5da34 to f319ed2 Jun 8, 2019

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2019

For now, I have followed and copied the same method for shadow pages as earlier controllers for real pages. As I thought that in future we'll be replacing the shadow ones back to original routes.

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2019

I tweaked frontend a bit and different cells being called from /questions and /dashboard were changed as per the info needed.

@aSquare14
Copy link
Member

left a comment

They look good to me but it would be a better practice to make a different file for css/scss and then import that in the erb.

@gautamig54

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Great work @CleverFool77 !!
It will be good if you can decrease the font size of the question in the cards as it will look absurd in case of long questions.
The heading and the cards are not aligned, can you do that as well.
Also, I think we need to add ellipsis here as well to avoid overflow of questions.

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

Great work @CleverFool77 !!
It will be good if you can decrease the font size of the question in the cards as it will look absurd in case of long questions.
The heading and the cards are not aligned, can you do that as well.
Also, I think we need to add ellipsis here as well to avoid overflow of questions.

Hi @gautamig54 I guess in design The question went to second line in card. And I guess we do flash notice to rite title not more than 6-7 words. As it shows that titile is bit too long message.

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

Screenshot from 2019-06-10 17-09-09

@CleverFool77 CleverFool77 force-pushed the CleverFool77:featured-question-section branch from f319ed2 to 6b91778 Jun 10, 2019

@CleverFool77 CleverFool77 added in-progress and removed review-me labels Jun 10, 2019

@CleverFool77 CleverFool77 force-pushed the CleverFool77:featured-question-section branch 2 times, most recently from 99ecdab to 5a8943e Jun 10, 2019

@CleverFool77 CleverFool77 added review-me and removed in-progress labels Jun 10, 2019

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

Hi @jywarren I just pushed the latest changes.
Thanks !!

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Looking good, added clarification to my inline question... @gautamig54 can you approve this PR too and both ping me (or another maintainer) when it's ready? Great work!!!

And, just for quick reference, can you point me at the URL where we'll be able to test this on stable when the time comes? Thank you!!!

@CleverFool77 CleverFool77 force-pushed the CleverFool77:featured-question-section branch from 5a8943e to 894505e Jun 10, 2019

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

hi @jywarren @gautamig54 Just pushed the latest changes as requested.
regarding the test, did you mean the main url /question_shadow ?

@gautamig54

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

@CleverFool77 Can you add a screenshot with testing question to be greater that two lines? Say 15 words?

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

Hi @gautamig54
Its there in flash notice while Filling up the form for posting qiestion that when title is exceeded more than 8-9 words. It gives message that way too long titile.

Screenshot from 2019-06-11 23-34-21

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

Hi @gautamig54 what do you say about my thought above?
Maybe we can forward the progress of this PR then.
Hi @jywarren
Can we have this PR merged first.
Regarding question size,
Do we need overflow fixing for the question size in frontend? Or a restriction in backend?
If yes, I'll fix this in next PR.
If no, then I guess it's usually understood for question title to be not too long?
And importantly how many words does the questions usually consist of?
As If fix is needed, then I might work on size of question based on words.

@sagarpreet-chadha
Copy link
Contributor

left a comment

The code is written very nicely! Good work :)
Regarding long title we can truncate the title by showing few words and then dots like : this is a question about ...

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

Seems like good idea sagarpreet. So maybe truncating the title of question to two-three lines with ellipsis in end?

@sagarpreet-chadha

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

Thank you and yes sounds great!

@gautamig54

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Restricting question length to just 8-9 words may be less. We should restrict it to atleast 20 words? I don't think 20 words is too long for a question.
Also, adding ellipsis(...) for longer question is the best suitable solution.

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

Sure gautami. Lets have this merged first. Till then I'm looking over the matter of overflow.
Thanks !!

@plotsbot

This comment has been minimized.

Copy link
Collaborator

commented Jun 13, 2019

1 Warning
鈿狅笍 There was an error with Danger bot鈥檚 Junit parsing: No JUnit file was found at output.xml
3 Messages
馃摉 @CleverFool77 Thank you for your pull request! I鈥檓 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鈥檛 be discouraged if you see errors 鈥 we鈥檙e here to help.
馃摉 This pull request doesn鈥檛 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)

5861-test_wiki.png

5861-test_tag_page.png

5861-test_login.png

5861-test_wiki_page_with_inline_grids.png

5861-test_questions.png

5861-test_stats.png

5861-test_tags.png

5861-test_people.png

5861-test_signup.png

5861-test_questions_shadow.png

5861-test_blog.png

5861-test_front_page_with_navbar_search_autocomplete.png

5861-test_viewing_the_dashboard.png

Learn about automated screenshots

Generated by 馃毇 Danger

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

The screenshot feature looks super cool 馃槷 馃槷 . Damnnnn 馃槏 !!!

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

Sure I'll add this for original question and shadow route for now.

@CleverFool77 CleverFool77 force-pushed the CleverFool77:featured-question-section branch from bdde304 to 5d609cb Jun 13, 2019

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

Hi @jywarren just pushed the latest changes with the addition of test screenshot for question and shadow route.
Thanks.

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Oooooh very cool!!!

@gautamig54

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

This looks great @CleverFool77! Let's get this one merged and move ahed with the recent questions section.

@jywarren jywarren merged commit 591b5ff into publiclab:master Jun 14, 2019

3 checks passed

codeclimate Approved by Jeffrey Warren.
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 Jun 14, 2019

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

rarrunategu1 added a commit to rarrunategu1/plots2 that referenced this pull request Jun 18, 2019

@CleverFool77 CleverFool77 deleted the CleverFool77:featured-question-section branch Jun 28, 2019

sagarpreet-chadha added a commit that referenced this pull request Jun 29, 2019
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
You can鈥檛 perform that action at this time.