-
Notifications
You must be signed in to change notification settings - Fork 869
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
[Brave News]: Better offline handling & UX tweaks #21507
Conversation
d42d117
to
e11e502
Compare
A Storybook has been deployed to preview UI for the latest push |
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.
strings
++
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 👍
{cards} | ||
<CaughtUp /> | ||
</> | ||
? errors[feed.error!] ?? <> |
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'm curious if this is a hacky way to do the same with
(feed.error && errors[feed.error]) ??
, as errors[null]
would be undefined anyway
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.
Exactly - errors has an exact list of keys it allows, but I wanted to treat it as a map which may or may not have the key. Just felt it read a bit nicer, but happy to change it 😄
Don't try and generate the feed when we have no items
a3d1f6d
to
32d01e7
Compare
A Storybook has been deployed to preview UI for the latest push |
Verification
|
step 0 |
step 3 |
step 4 |
step 5 |
step 6 |
step 7 |
step 8 |
step 9 |
---|---|---|---|---|---|---|---|
Case Two: brave/brave-browser#35155 - PASSED
(Followed steps from issue, above.)
Checked a couple iterations of the loading spinner and no longer see the square outline
example | example |
---|---|
2024-01-11_02h25_11.mp4
Case Three: brave/brave-browser#35154 - PASSED
Steps:
- disable system
Wi-FI
/ internet connectivity - install
1.63.109
- launch Brave
- open a new-tab page
- scroll down
- click on
Turn on Brave News
- re-enable Wi-Fi
- wait / click
Reload
/Refresh
etc. on the client-side
Confirmed after re-enabling Wi-Fi
and clicking Refresh
(or browser's Reload
button), the content loads/returns
step 0 |
example | step 6 |
example |
---|---|---|---|
Case Four: brave/brave-browser#35157 - PASSED
Steps:
- install
1.63.109
- launch Brave
- open a new-tab page
- scroll down
- click on
Turn on Brave News
- click
Customize
- click on
Brave News
- remove
Top Sources
- now add at least one (1) each for
Channels
and Publishers
Confirmed both the Publishers
and Channels
appear with at least one entry (each), at which point they get the collapse
/expand
toggle
default |
populated, with the expand /collapse arrows (CNN ) |
Cars |
---|---|---|
Encountered:
[Brave News]: Better offline handling & UX tweaks (#21507)
Resolves brave/brave-browser#35153
Resolves brave/brave-browser#35155
Resolves brave/brave-browser#35154
Resolves brave/brave-browser#35157
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Note: When testing this the company VPN should be off, as Brave can't detect the network state and requests hang (this happens for
fetch
requests from JavaScript too).