Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

Conversation

@Happy0
Copy link
Contributor

@Happy0 Happy0 commented Dec 27, 2017

… be able to see new content. Fixes #707.

@mmckegg
Copy link
Contributor

mmckegg commented Dec 27, 2017

Code looks pretty good! Gonna give this a try and see 👍

@mmckegg
Copy link
Contributor

mmckegg commented Dec 27, 2017

@Happy0

This seems to work! However, the warning could be a bit startling for a new user.

screen shot 2017-12-28 at 12 26 36 pm

Since this also serves as a welcome message, maybe this should provide links to a getting started guide?

Do you have any thoughts @ahdinosaur?

@Happy0
Copy link
Contributor Author

Happy0 commented Dec 28, 2017

haha yes, I agree (with regards to it being startling.)

By the way, I just moved the message under the 'write a public message' box so that it doesn't follow you as you scroll down =p.

I could probably do with a hand with the phrasing of the warning box / a good link to put at the bottom of it, and eh.. less startling styling :P.

@Happy0
Copy link
Contributor Author

Happy0 commented Dec 28, 2017

I'll update the pull request with:

Let you know when it's updated :P.

@Happy0
Copy link
Contributor Author

Happy0 commented Dec 28, 2017

Is this any better? @mmckegg

Front end stuff is not my strength. I'm trying though :P.

2017-12-28-003225_1600x900_scrot

The text is less intimidating. Do you think I should maybe go with a 'softer' colour scheme? Like the orange from the boxes on profiles warning you that someone might not be able to see your posts?

@Happy0
Copy link
Contributor Author

Happy0 commented Dec 28, 2017

Here's my latest attempt using the same styling for other warning boxes (on profiles):

2017-12-28-005831_1600x900_scrot

I'm going to stop spamming now :P.

@ahdinosaur
Copy link
Contributor

👍, i like the iterations @Happy0

@Happy0
Copy link
Contributor Author

Happy0 commented Dec 28, 2017

Thanks @ahdinosaur :).

I'm thinking I should add a similar box to channels that the user views too. When I first used patchwork, I wasn't sure if channels were 'more public' or not. I later realised they followed the same friend-of-a-friend model.

I suspect the user who unfollowed the pubs he joined that inspired this change ( %5TXFe2XWKvVOMWCelFF74a3t0OVhPkYi4HGpeMqSlpg=.sha256 ) might have thought he could 'lurk' and just view channels without friending anybody.

Similarly, somebody who was onboarded over LAN will be able to see channels, but if they don't follow anybody they won't see new content on channels when they're off the LAN network.

The channel box would have the text:

'
You are Not Following Anyone.

You will not see any new content in this channel until you follow someone.'
'

Do you think this is worth adding too? (cc: @mmckegg )

@Happy0
Copy link
Contributor Author

Happy0 commented Dec 28, 2017

2017-12-28-134730_1600x900_scrot

They call me spammy0 ;x

@Happy0 Happy0 force-pushed the not_following branch 2 times, most recently from fe30165 to 88eda56 Compare December 28, 2017 14:01
locales/en.json Outdated
"Subscribed": "Subscribed",
"Write a message in this channel": "Write a message in this channel",
"mentioned this channel": "mentioned this channel",
"You are not Following Anyone": "You are not Following Anyone",
Copy link
Contributor

Choose a reason for hiding this comment

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

why is "Following Anyone" capitalized?

h('h1', i18n('You are not Following Anyone')),
h('p', i18n('You may not be able to see new channel content until you follow some users or pubs.')),
h('p', [i18n("For help, see the 'Getting Started' guide at "),
h('a', {href: 'https://www.scuttlebutt.nz/'}, 'https://www.scuttlebutt.nz/')]
Copy link
Contributor

@ahdinosaur ahdinosaur Jan 2, 2018

Choose a reason for hiding this comment

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

if we want to link directly to the "Getting Started" page, it's now available at: https://scuttlebutt.nz/getting-started.html

]
}

function noVisibleNewPostsWarning() {
Copy link
Contributor

@ahdinosaur ahdinosaur Jan 2, 2018

Choose a reason for hiding this comment

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

this is mostly repeated from modules/page/html/render/channel.js, is it possible (and desirable) to extract out the similar bits into a common module? same for the mcss, which could be a mixin?

Copy link
Contributor Author

@Happy0 Happy0 Jan 2, 2018

Choose a reason for hiding this comment

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

At the time, and thought that trying to abstract it maybe didn't benefit us much because I'd just be replacing 'when' with another function call. However, now that you mention it, I notice that most of the HTML is shared too.

I guess the explanation of why the warning is being shown 'You won't see new public posts' or 'you won't see new channel posts' can be parameterised, which would make it more worthwhile.

Next time I work on the branch, I'll push something like:

`
function renderWarning(conditionObs, explanation) {
var content =
h('div', {classList: 'NotFollowingAnyoneWarning'}, h('section -notFollowingAnyoneWarning', [
h('h1', i18n('You are not Following Anyone')),
h('p', explanation)),
h('p', [i18n("For help, see the 'Getting Started' guide at "),
h('a', {href: 'https://scuttlebutt.nz/getting-started.html'}, 'https://scuttlebutt.nz/getting-started.html')]
),
]))

  return when(conditionObs, content)

}`
I'll check out the mixin thing you linked me too =].

Cheers for the review :D.

@Happy0
Copy link
Contributor Author

Happy0 commented Jan 2, 2018

@ahdinosaur I've created a new module (feed.html.followWarning) for the warning rendering code shared by public.js and channel.js . @mmckegg : let me know if this breaks the naming convention for modules - it was the most appropriate place I could think to put it.

I've also moved the shared code for rendering the warning to a $distance-warning mixin. I'd copied and pasted the css from the profile-header.mcss distance-warning class (as I thought it was thematically fitting, as it's a warning about following distance.)

Do these changes make sense to you?

@mmckegg
Copy link
Contributor

mmckegg commented Jan 2, 2018

This is looking pretty good @Happy0!

I will review in depth and (hopefully) merge it next time get around to doing some patchwork dev!

Thanks @ahdinosaur for the code reviews!

@Happy0
Copy link
Contributor Author

Happy0 commented Jan 3, 2018

Cheers m80

@mmckegg mmckegg merged commit be34adb into ssbc:master Jan 29, 2018
@mmckegg
Copy link
Contributor

mmckegg commented Jan 29, 2018

Okay, merged! Will be in the next release 🎂

@Happy0
Copy link
Contributor Author

Happy0 commented Jan 30, 2018

\o/ :D

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants