Discovery UI #2477
No reviewers
Labels
No labels
accessibility
app-parity
area: creator
area: daemon
area: design
area: devops
area: discovery
area: docs
area: installer
area: internal
area: livestream
area: performance
area: proposal
area: reposts
area: rewards
area: search
area: security
area: subscriptions
area: sync
area: ux
area: viewer
area: wallet
BEAMER
channel
comments
community PR
consider soon
core team
css
dependencies
electron
Epic
feature request
first-timers-only
good first issue
hacktoberfest
help wanted
hub-dependent
icebox
Invalid
level: 0
level: 1
level: 2
level: 3
level: 4
merge when green
needs: exploration
needs: grooming
needs: priority
needs: repro
needs: tech design
notifications
odysee
on hold
playlists
priority: blocker
priority: high
priority: low
priority: medium
protocol dependent
recsys
redesign
regression
resilience
sdk dependent
Tom's Wishlist
trending
type: bug
type: discussion
type: improvement
type: new feature
type: refactor
type: task
type: testing
unplanned
windows
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: LBRYCommunity/lbry-desktop#2477
Loading…
Reference in a new issue
No description provided.
Delete branch "discovery"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Changes
FileListItemNotes
.scssfiles for now. I need to make a PR to github.com/lbryio/color before the styles are complete.Attempted to run the
discoverbranch and encountered errors using the UI. I'm going to do a code review, but I'm unable to do a UI/UX review.Maybe
tag-chooserortag-selectoror something like this?This component should also be named the same or similar as the outer class name, rather than
DiscoveryFirstRun.Why delete and toggle? Shouldn't it be just toggle or delete and add?
This will presumably change with @seanyesmunt's work
I would prefer we simply toggle classes and have cleaner CSS than using negation rules.
I will be making a number of changes to this branch today as I wire up redux.
The idea was that sometimes you might want to create a tag, but not follow it. ex: on the publish page
Creating/deleting a tag removes it from the
knownTags. Deleting a tag also removes it fromfollowedTags.In this flow, you might want to delete recommended tags that you know you won't use again. Or use tags for your content that you never plan to follow.
ref's ftw!@ -12,6 +12,7 @@<meta property="og:description" content="All your favorite LBRY content in your browser." /><meta property="og:image" content="/og.png" /><!-- @endif --><meta name="viewport" content="width=device-width, initial-scale=1" />@kauffj You said not to build a mobile layout, but you didn't say not to create a new desktop layout that just happens to look good on mobile too.
Both of these can be moved into their respective components and use persisted local state to manage if we should show them or not.
I need to add the drop to delete functionality. You can ignore this file for now. Waiting to hear back on this:
https://github.com/atlassian/react-beautiful-dnd/issues/1331
@ -0,0 +1,69 @@// @flowNeed to cleanup this file
Will update these and add constants
Not really a fan of
sort. This is the item on the right side of the header. Currently the two pages that use this are the homepage (trending/best/new) and search feedback (satisfied with results?)Sort isn't really a good name.
I'm not done but I have to go home.
Completely/somewhat out of scope, but I think it'd be cool if we had a tool that reported on changes to the bundle size whenever
package.jsonchanges.maybe
main-wrapper-inner?It looked like there are rules still targetting this class but I did not see it added anywhere else...
I think using this as a flag to both trigger default behavior and as the string key for storing state is mixing purposes.
Also, are passing
headeranddefaultHeadermutually exclusive? If so, maybe omittingheadercan trigger default behavior and we can be explicit about the key being for state storage.Probably should just be named
file_sort.I believe it's possible to create a
Claimintersection type that includes both of theseOut of scope, but in case you're doing any relevant refactoring, this is dying / should die and should instead be a set of
0...nobjectionable attributes, each of which the user can opt out of displaying individually.Why stop propagation here?
What about privileging tags I follow? Also, the slice should be sorted or otherwise consistently ordered (one claim may have tags
apple, banana, orange, while the other hasorange, banana, apple).@ -91,0 +19,4 @@return (<main className={classnames('main', className)}>{/* @if TARGET='app' */}{showUpgradeButton && (👏
IMO this isn't help text and we should reserve
helpfor content that is specifically aiding/teaching usage (please bring up on standup if you disagree and/or think we haven't been doing this). I think it is better to add a separate style called "empty" or similar for this type of styling. It's fine if they're even styled identically for now, but using distinct names will ease later style revisions.@ -54,12 +41,8 @@ class RewardSummary extends React.Component<Props> {navigate="/$/rewards"label={hasRewards ? __('Claim Rewards') : __('View Rewards')}boooo
it's fine though, I do it too
Mildly better if anything passing a class name could simply wrap the component or if there are distinct, enumerable styles to allow passing rather than this pattern (anything passing this is now complected with the component).
I don't understand this comment but happy to help if I can
@ -0,0 +1,69 @@// @flowI didn't read it
What if I'm not following any tags?
Make sure to put "removed vaguely threatening message from transaction list" in the release notes
I disagree with this. Let me learn that it is possible and what it looks like. Put the form and/or buttons in a non-interactive state if we think it's poor UX to display a form that cannot be submitted.
This probably shouldn't be an h1
Even if it is an
h1, just give it a class likefile-list__header-textor whatever📈
I agree it was pretty messy. I added
persistedStorageKeyand use the default header if noheaderis passed in. If noperistedStorageKeyis passed in, it will share state with other instances of the component that don't pass it in.I added a key for every component right now.
We don't need to
mostly done
so if I pass nothing or false, it still persists in a global namespace? IMO make all persisting here explicit and do not persist if a value isn't passed
headerAltControls?headerSecondaryControls? or same names without the word header?similar to my feedback on the lbry-redux PR, I think using the name trending repeatedly is a mild mistake, because it does things beyond trending
I would call the entire component FileListDiscover and call the above value just
urisor something like thisShould this just be a component? Or at least not inlined? Same for the sort parameter below
I'd make a capitalize function since you repeat this four times. It can simply be inlined in the file if it's only used here.
Is this used?
Above comment still stands, especially since you're only using it in one place
missing i18n
out of scope, but this should be a way of saying "max" instead of 8
--no-verifyMildly uncertain about the pattern of relying on the local persisted state of
<TagsSelect>being closed to not re-render this. I suppose that is fine though.@ -1,45 +1,42 @@// @flowif I notice another one I won't comment since I know this will be linted eventually
@ -43,0 +35,4 @@</section></div>)}</React.Fragment>Per comments in Slack, I'm fine with dropping the history, at least temporarily, if that makes this easier/cleaner
why does the capitalization switch?
@ -0,0 +17,4 @@doToggleTagFollow,} = props;const urlParams = new URLSearchParams(search);TIL
Yeah I wasn't really sure about this. Another option is to always show the injected item. It could be in the third/fourth position if there are more than four items, otherwise, just show at the end.
I figured we would have another on the subscriptions page somewhere.
@ -43,0 +35,4 @@</section></div>)}</React.Fragment>It would, and I'm fine with that for now. I'll remove it/make the other changes in the next PR
Because someone stole my computer and changed the titles without me knowing.
👍 I pulled it out. Possibly there should be
FileListHeadercomponent, but I think it helps to keep this logic here, since all of this is related.