-
Notifications
You must be signed in to change notification settings - Fork 911
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
Provide a summary view of recent diaries #5103
base: master
Are you sure you want to change the base?
Provide a summary view of recent diaries #5103
Conversation
Failed test is out of scope of this PR, as there was no change or impact related to the test that was failed. |
This PR also partially refers to #3721 (changes HTML view of the diary entries list, but doesn't change RSS) |
I haven't tested this code yet, though it seems that toggling a diary entry would either show the whole text, or only the title. I believe #3887 talked about showing the first few n lines of the body, with an option to further expand the body of the diary. Not seeing any text at all except for the title is hiding a lot of content, and a user needs to click on every single diary entry to see what's behind. This seems to me rather cumbersome. Would this be possible to implement? If that's already done, that would be even better! |
Yes this starts with all entries collapsed to just their headings. It also fails if you have CSP enforcement enabled because bootstrap is using content_security_policy(:only => :index) do |policy|
policy.img_src(*policy.img_src, "data:")
end |
@mmd-osm Yes, it is possible. I made this change according to the last screen mentioned in the Issue #3887. I'll change this PR to show part of the content even when it is collapsed. @tomhughes I'll take a look if there is any way to make it work without custom code. |
a8a91db
to
815cb26
Compare
@mmd-osm I updated PR so that now @tomhughes I implemented solution suggested by you. I found some issues opened about Bootstrap CSP errors (twbs/bootstrap#25394), but even Bootstrap 5 mentions that some of their functionality uses Thank you for reviewing |
I guess the question is do we really want/need the collapse and expand functionality or is it better just to show a truncated post and let people click through if they want to see the whole thing, which I think is a more typical way to handle this sort of thing. |
@tomhughes Without a collapse functionality, code will be simpler, there will be no need of additional custom JS and there will be no CSP errors. From the coding point of view, it's much better solution. If we don't sacrifice any important UX, I more than agree with only truncation without collapse-expand button. |
I created a separate PR for the truncated version of the entry #5121. If we decide to truncate diary entries instead of showing summary of them, I'll close this PR. |
This PR addresses "Provide a summary view of recent diaries" issue mentioned in the #3887
Bootstrap Accordion was added to the diary entries list pages. Now the content of the diary entry will be collapsed until toggle button is clicked. View of the diary entry details (show) page remains the same. Fixes #3887
Screenshots: