-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left comments on your changes. The modifications to the navigation bar and settings represent a good start, but I'm thinking we take care of #13 first (I can take that on) so that we don't have to modify the post.html/duplicates.html/user.html templates.
let query: Vec<&str> = query_str.split('&').collect::<Vec<&str>>(); | ||
|
||
for param in query.into_iter() { | ||
for param in query_str.split('&') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this. I'm a Rust neophyte so any way my code can be optimized is good by me.
templates/base.html
Outdated
@@ -30,7 +30,7 @@ | |||
<nav> | |||
<div id="logo"> | |||
<a id="libbacon" href="/"><span id="lib">lib</span><span id="bacon">bacon.</span></a> | |||
<span id="version">v{{ env!("CARGO_PKG_VERSION") }}</span> | |||
<span id="version">v{{ env!("CARGO_PKG_VERSION") }}{% if crate::utils::sfw_only() %} - 💼{% endif %}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The briefcase to communicate the status of the instance is an excellent idea, but I'm not crazy about the hyphen.
On the briefcase itself, can we get a hover-over help tip to explain what the suitcase means?
templates/duplicates.html
Outdated
@@ -23,8 +23,12 @@ | |||
<!-- DUPLICATES --> | |||
{% if post.num_duplicates == 0 %} | |||
<span class="listing_warn">(No duplicates found)</span> | |||
{% else if post.flags.nsfw && prefs.show_nsfw != "on" %} | |||
{% else if true %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else if true
is unnecessary. Move the if-else blocks that are inside this block into the larger one.
templates/duplicates.html
Outdated
{% else if post.flags.nsfw && prefs.show_nsfw != "on" %} | ||
{% else if true %} | ||
{% if crate::utils::sfw_only() %} | ||
<span class="listing_warn">SFW-only mode enabled.</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I'd like to implement #13 so that any attempt to view NSFW content when NSFW-mode is disabled results in the wall, the same template rendered for quarantined and banned subs. When that goes in, I don't think we'll need to handle the case of utils::sfw_only()
at all since the handlers for posts and duplicates respectively will render the wall templates and not the templates containing post content (post.html and duplicates.html).
templates/search.html
Outdated
@@ -58,6 +58,9 @@ | |||
{% endif %} | |||
|
|||
{% if all_posts_hidden_nsfw %} | |||
{% if crate::utils::sfw_only() %} | |||
<span class="listing_warn">All posts are hidden because they are NSFW. This is an SFW-only instance.</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer: The results for your query are all NSFW. Since this is a SFW-only instance, they are hidden.
templates/settings.html
Outdated
@@ -116,6 +118,10 @@ | |||
<div id="settings_note"> | |||
<p><b>Note:</b> settings and subscriptions are saved in browser cookies. Clearing your cookies will reset them.</p><br> | |||
<p>You can restore your current settings and subscriptions after clearing your cookies using <a href="/settings/restore/?theme={{ prefs.theme }}&front_page={{ prefs.front_page }}&layout={{ prefs.layout }}&wide={{ prefs.wide }}&post_sort={{ prefs.post_sort }}&comment_sort={{ prefs.comment_sort }}&show_nsfw={{ prefs.show_nsfw }}&blur_nsfw={{ prefs.blur_nsfw }}&use_hls={{ prefs.use_hls }}&hide_hls_notification={{ prefs.hide_hls_notification }}&subscriptions={{ prefs.subscriptions.join("%2B") }}&filters={{ prefs.filters.join("%2B") }}">this link</a>.</p> | |||
{% if crate::utils::sfw_only() %} | |||
<br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<br />
templates/subreddit.html
Outdated
@@ -17,7 +17,9 @@ | |||
|
|||
{% block body %} | |||
<main> | |||
{% if !is_filtered %} | |||
{% if all_posts_hidden_nsfw && crate::utils::sfw_only() %} | |||
<p><span class="listing_warn">All posts are hidden because they are NSFW. This is an SFW-only instance.</span></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All posts for this NSFW subreddit are hidden because this is a SFW-only instance.
(But per my comment on templates/duplicates.html:26 we won't need to check for this in the template if the item handlers in post.rs/duplicates.rs return a new wall template; see #13.)
templates/user.html
Outdated
@@ -33,16 +33,19 @@ | |||
</form> | |||
|
|||
{% if all_posts_hidden_nsfw %} | |||
<center>All posts are hidden because they are NSFW. Enable "Show NSFW posts" in settings to view.</center> | |||
{% if crate::utils::sfw_only() %} | |||
<span class="listing_warn">All posts are hidden because they are NSFW. This is an SFW-only instance.</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posts for this user, all NSFW, are hidden because this is a SFW-only instance.
src/utils.rs
Outdated
// Set the LIBBACON_SFW_ONLY environment variable to anything, to enable SFW-only mode. | ||
// If environment variable is set, this bool will be true. Otherwise it will be false. | ||
pub fn sfw_only() -> bool { | ||
env::var("LIBBACON_SFW_ONLY").is_ok() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The env var should beI've changed my mind about this; this isn't a user setting, so we shouldn't call it a "default." I agree withLIBBACON_DEFAULT_SFW_ONLY
to be consistent with the other setting names.FERRIT_SFW_ONLY
.- If we're going to sidestep the settings generation in
utils::setting()
, we should justify why in a comment. Obviously we don't want this user-configurable, which is why we're checking for the variable's existence here and not inutils::setting()
, but we should document this. Also: - Use
///
. (A future task I want to take on is adding and improving documentation that exists.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new variable should also be added to the Heroku manifest at app.json
.
I've opened up #25, which resolves #13. I'd like that to go in before we take care of #23. When #25 goes in, we can just change the message in the landing page to state that the content is forbidden from view since the instance is SFW-only, and we can modify the condition governing whether to render the landing page based on that too. For example, this line in src/post.rs: if setting(&req, "show_nsfw") != "on" && post.nsfw { would become if (utils::sfw_only() || setting(&req, "show_nsfw") != "on") && post.nsfw { |
@sigaloid: There've been a significant number of changes since this PR was opened so I'm thinking you should redo this. I'll refer to my previous two comments (1, 2) as to what I think we should to with respect to both the templates and the logic that determines when to return the NSFW landing page. |
Hi, I'll redo this PR. Apologies for the stagnation, I've been dealing with a bios issue on my PC. Hoping to have this done over the weekend. |
Hi @Daniel-Valentine, I've redone this PR and force-pushed it. It now renders the NSFW landing page. I also changed the logic to show the instance error if it's a SFW instance, regardless of whether the setting is enabled or not - I need to add the logic to hide the NSFW settings to the settings page, though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I've also changed the title of this PR to reflect the new env var name with the project name change.
src/utils.rs
Outdated
// Set the LIBBACON_SFW_ONLY environment variable to anything, to enable SFW-only mode. | ||
// If environment variable is set, this bool will be true. Otherwise it will be false. | ||
pub fn sfw_only() -> bool { | ||
env::var("LIBBACON_SFW_ONLY").is_ok() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new variable should also be added to the Heroku manifest at app.json
.
Fixes #23
There is also a briefcase emoji displayed in the header as well as a message in the settings menu (hiding the NSFW-related settings).