Newnew #2144
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#2144
Loading…
Reference in a new issue
No description provided.
Delete branch "newnew"
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?
Created a new PR for #2102 because that had so many comments.
PR Checklist
PR Type
What kind of change does this PR introduce?
Fixes
I borked https://github.com/lbryio/lbry-desktop/pull/2053 while attempting to have a sane rebase. This PR refactors the CSS and makes semantic changes in the rendered HTML.
Closes #1976.
What is the current behavior?
Nothing the average user would notice, aside from styling.
What is the new behavior?
New hotness. Sass is (more) organized and easier to manage. It still isn't perfect but it's close.
You might be starting to dislike me, but some more comments.
I think these are mostly minor alignment/consistency issues. The code is looking way better (re media-tile media-card).
I think the biggest outstanding issue is the icons not using the
Iconcomponent.I know there will always be stuff that can be fixed so I don't want to have this sit in review forever. I think we can merge this after these issues are addressed then fix any other stuff individually.
@ -265,4 +254,3 @@className="btn--arrow"disabled={!canScrollNext}onClick={this.handleScrollNext}icon={ICONS.ARROW_RIGHT}The spacing between the
border-bottomand the content is not consistentIf we are switching to a
ul(which I think is a good idea), we should wrap theFileCardin anliThere used to be a min-height on this for cases like the empty cards on line 307. Currently suggested subscriptions don't render any cards until something is loaded because there is no height. Maybe the placeholders just need a height?
@ -62,3 +74,3 @@{!isResolvingUri && (<React.Fragment><div className="file-tile__title"><div className="media__title">This is misaligned on the search page
We should use this component with the wallet balance in the header.
@ -45,0 +42,4 @@(claimed ? (<span><Icon icon={ICONS.CHECK} /> {__('Reward claimed.')}</span>This style needs to be applied to the
"Humans Only"header found inpage/rewards/view.jsxI would try clearing your auth token to go through the verification process again. The
"Human Proofing"email collection form looks off, I think it's just missing the new styles.@ -54,2 +48,3 @@'nav__link--active': active,className={classnames('navigation__link', {'navigation__link--active': active,})}Shouldn't this stay as a
ul?This is going to error because of no
iconsimportForm imports I've been trying to follow the pattern of:
Not a big deal though
@ -17,0 +14,4 @@<h2 className="card__title">{__('Balance')}</h2><p className="card__subtitle">{__('You currently have')}</p></header>I think we might need to make some changes here. Since the balance is much larger, it takes up way more horizontal space if your balance has a lot of decimals.
Both of these are on the smallest possible screen width
@ -134,0 +137,4 @@type="text"/></FormRow>)}When navigating to a channel page, there can be some content jank if it loads quickly. It's happening because of the style changes in
page/show/view.jsx. I don't think we need those. Before it was really nice how the channel stayed in the same place when everything loaded. That call usually pretty quick so I think we should keep it.https://giphy.com/gifs/Zw7a22HKncKIaolawh
This should probably be capitalized since it's on a new line now.
@ -250,0 +207,4 @@prepareEdit(claim, editUri);navigate('/publish');}}/>If we are removing the wrapper on this, we should probably add some sort of max-height, then add a background so the thumbnail, content doesn't get huge. Most thumbails aren't great on large screens, and this will only be exacerbated on larger screens.
Alignment is off on this
@ -97,0 +76,4 @@min={10}max={1000}value={resultCount}onChange={this.onSearchResultCountChange}The placeholder seems broken for this top result on the search page. For channels it looks fine.
These should stil use the
IconcomponentI think is causing alignment issues. See the transactions table. The
Exportbutton isn't aligned with the filter dropdown because it's centered vertically. Maybe we could remove this and give buttons aheight?Eh, it's not a big deal. Your solution is probably better.
This doesn't seem to work for labels on inputs. See the
Send & Recievepage.These should all use the
Iconcomponent@ -42,0 +34,4 @@width: calc(var(--header-height) * 3 + 1px);}}Should these be variables? If it's specific to header stuff maybe just add a comment because it's confusing from a glance.
@ -0,0 +1,210 @@// Generic html styles used accross the AppI'm not able to highlight anything in the app. We shouldn't be preventing this.
@ -265,4 +254,3 @@className="btn--arrow"disabled={!canScrollNext}onClick={this.handleScrollNext}icon={ICONS.ARROW_RIGHT}This is probably due to the additional icon div that appears when you have something downloaded. If you scroll, do you see icons?
@ -54,2 +48,3 @@'nav__link--active': active,className={classnames('navigation__link', {'navigation__link--active': active,})}No, there's already a
ulsurrounding the account links. I don't think Explore and Subscriptions need to belis.@ -0,0 +1,210 @@// Generic html styles used accross the AppDo we need the ability to highlight everything? Titles, markdown content, wallet addresses and so on make sense but not things like transaction fees.
@ -265,4 +254,3 @@className="btn--arrow"disabled={!canScrollNext}onClick={this.handleScrollNext}icon={ICONS.ARROW_RIGHT}Yeah it is. Should this always keep a space for that div? If you start downloading something, then go to the home page before it starts, it jumps when the icon is added.
@ -0,0 +1,210 @@// Generic html styles used accross the AppProbably not everything, but I don't think we should prevent highlighting if users want to. It's a lot more work to have to opt into something if we want it to be highlightable.
@ -54,2 +48,3 @@'nav__link--active': active,className={classnames('navigation__link', {'navigation__link--active': active,})}It is a list of items though, even if it's only two. I don't think we lose anything from adding it, but we gain accessibility improvements, which we will need to focus on more for web/tv uis to better handle keyboard/remote navigation.
@ -265,4 +254,3 @@className="btn--arrow"disabled={!canScrollNext}onClick={this.handleScrollNext}icon={ICONS.ARROW_RIGHT}I'll add a height to it so the jumping doesn't happen.
I thought this was going to be resolved after this PR was merged?
If we are adding new icons I would prefer they are implemented using the
Iconcomponent, just so it doesn't slip through the cracks.We had a whole conversation about this. It'd require creating a new repo to replicate the react-feathers module. Should this be done now, or later?
It wouldn't require a new repo, that might be a better long-term solution. This could be implemented by something like this:
Then we have all of these new custom icons in one place.
I wish this was brought up when we had that conversation.
My fault. Sorry about that, I thought we were on the same page.
@ -17,0 +14,4 @@<h2 className="card__title">{__('Balance')}</h2><p className="card__subtitle">{__('You currently have')}</p></header>I thought we weren't worrying about making things responsive.
@ -17,0 +14,4 @@<h2 className="card__title">{__('Balance')}</h2><p className="card__subtitle">{__('You currently have')}</p></header>Why are we showing all these decimal points? We don't in the header bar. We don't in the hover text either.
This does not look good on the
<span/>surrounding the amount. You have ellipsis and extra space to add "LBC" in an::afterpseudo-element.Another option is to have the numbers fade (to replace the ellipsis).
Let me know which direction you want me to go in.
@ -17,0 +14,4 @@<h2 className="card__title">{__('Balance')}</h2><p className="card__subtitle">{__('You currently have')}</p></header>We aren't worrying about mobile design, but the minimum app width still needs to look good.
We should show the full amount here, it's the only place you can see it. I think it's fine to show an estimate in the header because it goes to this page when you click on it, and shows the full amount in the hover text. It's a bug that we show an estimate when hovering over the large balance (I guess we don't even really need that though).
What about just dropping the
font-sizedown to4rem?@ -17,0 +14,4 @@<h2 className="card__title">{__('Balance')}</h2><p className="card__subtitle">{__('You currently have')}</p></header>I'll make it
3.5remso the Rewards box has more breathing room at950px.@ -42,0 +34,4 @@width: calc(var(--header-height) * 3 + 1px);}}I don't get the confusion. This is how responsive stuff works. These rules are inside a header element.
@ -97,0 +76,4 @@min={10}max={1000}value={resultCount}onChange={this.onSearchResultCountChange}Could you add photos of both?
@ -97,0 +76,4 @@min={10}max={1000}value={resultCount}onChange={this.onSearchResultCountChange}It's only before the content is loaded. Type in
@jiggytomin the search bar. There is no height while it's being fetched.@ -97,0 +76,4 @@min={10}max={1000}value={resultCount}onChange={this.onSearchResultCountChange}I must have a great connection because I don't see these hitches. I'm adding min-heights to things now.
@ -97,0 +76,4 @@min={10}max={1000}value={resultCount}onChange={this.onSearchResultCountChange}Try clearing cache then try again. It might be already fetched. Or just search for another channel.
@ -42,0 +34,4 @@width: calc(var(--header-height) * 3 + 1px);}}If 600px is a global width that will trigger ui changes, it should be a variable. But if only the header is changing at 600px then a comment explaining that will help.
@ -42,0 +34,4 @@width: calc(var(--header-height) * 3 + 1px);}}Ah gotcha. I'll add comments to all media queries then.