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

add message to say content is hidden according to settings (2703) #3038

Merged
merged 2 commits into from
Oct 16, 2019

Conversation

tombiju
Copy link
Contributor

@tombiju tombiju commented Oct 14, 2019

PR Checklist

Please check all that apply to this PR using "x":

  • I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
  • I have checked that this PR does not introduce a breaking change
  • This PR introduces breaking changes and I have provided a detailed explanation below

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting)
  • Refactoring (no functional changes)
  • Documentation changes
  • Other - Please describe:

Fixes

Issue Number: 2703 (#2703)

What is the current behavior?

  • No warning message that content might be hidden

What is the new behavior?

  • A message shows up in the UI near the top notifying the user that there settings might hide some content, and there is a link to go to the settings page and modify them.

  • Although the ticket only asked for mature tags, the message is not too much in the users face so I thought it would be good to have it in the landing discover page, during tag searches, and during regular text searches:

image

image

image

Other information

@neb-b neb-b self-requested a review October 14, 2019 14:21
@lbry-bot lbry-bot assigned neb-b and unassigned neb-b Oct 14, 2019
Copy link

@neb-b neb-b left a comment

Choose a reason for hiding this comment

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

This is looking great! Just a couple small comments. Can you also add a changelog entry to this file?
https://github.com/lbryio/lbry-desktop/blob/master/CHANGELOG.md

return (
<div className="section--padded section__subtitle">
<Icon icon={ICONS.EYE_OFF} />
{__(' Content might be hidden on this ' + type + ' because of your ')}
Copy link

Choose a reason for hiding this comment

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

Please change this to:

<I18nMessage tokens={{ type, settings: <Button button="link" label={__('settings')} href="/$/settings" /> }}>
  Content may be hidden on this %type% because of your %settings%
</I18nMessage>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this 👍

@@ -92,6 +95,7 @@ export default function ClaimList(props: Props) {
<div className={classnames('claim-list__header', { 'claim-list__header--small': type === 'small' })}>
{header}
{loading && <Spinner type="small" />}
{obscureNsfw && <HiddenNsfw type="page" />}
Copy link

Choose a reason for hiding this comment

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

I'm not sure this should live in this component. I don't think this should be shown on the home page or your list of publishes. Probably only in page/tags/view.jsx, maybe we could pass a prop down from there?

I like the styling on this though!

Copy link
Contributor Author

@tombiju tombiju Oct 15, 2019

Choose a reason for hiding this comment

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

Moved it to the tags view! Also I edited the icons .scss to give a little bit of space between the icon and the message which are mashed together now that its in the I18nMessage component.
Now this message should only appear when viewing tag results.

@tzarebczan
Copy link
Contributor

@tombiju thanks for the PR, your first!

Can we show you some appreciation for the contribution?

Also, we're giving away some Hacktoberfest bonuses and goodies for this month. We'll get it all figured out after you shoot us an email after this is reviewed/merged.

@kauffj
Copy link
Member

kauffj commented Oct 15, 2019

@seanyesmunt @tombiju maybe we could brainstorm some other ways of displaying this? I think adding this is a definitely a valuable addition, but it is noisy to see this frequently if you are actively and purposefully blocking channels.

I would be inclined to put it at the bottom of the results, but with infinite scroll this is tricky.

If we cannot come up with a solution other than putting it in the top, I think we should reduce it to an icon with text on mouseover and/or make it collapsable to an icon so you don't have to keep seeing it.

@neb-b
Copy link

neb-b commented Oct 15, 2019

@kauffj This is only for viewing mature tag pages with the setting off. Blocked channels are completely hidden from the UI.

@tombiju This is closer but we should only show this setting if you are on the tag page and it's a mature tag. We have a list of the current tags we track here:
https://github.com/lbryio/lbry-redux/blob/master/src/constants/tags.js#L16

@@ -109,6 +111,7 @@ function ClaimListDiscover(props: Props) {
(personalSort === SEARCH_SORT_CHANNELS && subscribedChannels.length) ||
(personalSort === SEARCH_SORT_YOU && !!tags.length) ||
personalSort === SEARCH_SORT_ALL;
const hasMatureTags = tags.some(t => MATURE_TAGS.includes(t));
Copy link
Contributor Author

@tombiju tombiju Oct 15, 2019

Choose a reason for hiding this comment

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

@seanyesmunt @kauffj Just added this in to see if the tag is mature, so the message will be shown accordingly and it shouldn't be too obtrusive anymore!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants