Desktop News customize: unfollow removes follow #15499

petemill · 2022-10-18T02:29:43Z

…and does not block ontent. follow / unfollow button text instead of heart icon

There will be a further follow-up to bring the exact design for the Follow / Unfollow button as well as channel icons. This can happen once Leo is available as a dependency.

Resolves brave/brave-browser#26090

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

On issue

fallaciousreasoning

LGTM, I've left a few comments, and one question about how the hover/focus works for the cards (for me, when I click on them it focuses the button and moving the mouse off the card means the button stays visible until I click something else).

Approved because it's fairly minor, and definitely an improvement.

fallaciousreasoning · 2022-10-18T02:35:59Z

components/brave_new_tab_ui/components/default/braveToday/customize/ChannelCard.tsx

 interface Props {
-  channelId: string
+  channelName: string


Good call, in my original iteration I thought channels might have a bit more info - nice to keep things simple.

Yeah we still need to do translation and images and I thought I was going to do that in this PR, but this still makes sense I suppose for now.

fallaciousreasoning · 2022-10-18T02:36:51Z

components/brave_new_tab_ui/components/default/braveToday/customize/DiscoverSection.tsx

@@ -15,7 +15,6 @@ interface Props {

 const Container = styled(Flex)`
  padding: 16px 0;
-  cursor: pointer;


Right, I should've removed this when I got rid of the channel pages

fallaciousreasoning · 2022-10-18T02:40:09Z

components/brave_new_tab_ui/components/default/braveToday/customize/ChannelCard.tsx

@@ -20,30 +26,27 @@ const Container = styled(Flex) <{ backgroundColor: string }>`
  color: white;
  position: relative;

-  :hover {
-    opacity: 0.8;
+  &[data-channel-card-is-followed=true] {


Okay, so I found this a bit weird. When I click on the button, it sets the :focus-within pseudo element so the button stays visible, even when I move the mouse outside. Then, when I click on something else, the button disappears.

Fixed with :has(:focus-visible)

fallaciousreasoning · 2022-10-18T02:41:42Z

components/brave_new_tab_ui/components/default/braveToday/customize/FeedCard.tsx

+
+  &[data-feed-card-is-followed=true] {
+    &:not(:hover, :focus-within) ${StyledFollowButton} {
+      opacity: 0;


Same here, it just feels a bit odd. Maybe this is what we want though.

fallaciousreasoning · 2022-10-18T02:41:50Z

components/brave_new_tab_ui/components/default/braveToday/customize/FeedCard.tsx

 const Name = styled.span`
  font-size: 14px;
  font-weight: 600;
 `

-const Pulse = keyframes`


fallaciousreasoning · 2022-10-18T02:42:57Z

components/brave_new_tab_ui/components/default/braveToday/customize/FeedCard.tsx

@@ -132,7 +81,7 @@ export function DirectFeedCard (props: {
 }) {
  const [loading, setLoading] = useState(false)
  return <Flex direction="column" gap={8}>
-    <Card backgroundColor={getCardColor(props.feedUrl)}>
+    <Card backgroundColor={getCardColor(props.feedUrl)} data-feed-card-is-followed={true}>


Do we need to do this here? I think if the direct feed is followed we will hide it and just show a channel card instead

Only because it's using the same styles, hence the true instead of a variable

fallaciousreasoning · 2022-10-18T02:43:57Z

components/brave_new_tab_ui/components/default/braveToday/customize/FollowButton.tsx

-        : <>
-          {HeartOutline} {getLocale('braveNewsFollowButtonNotFollowing')}
-        </>}
+    <Button {...rest} scale='tiny' isPrimary={!following}>


👍 nice to see the button getting less loud

We can replace this with Leo's button when we have that available and maybe use that opportunity to clean all this up a bit and reduce repetition

fallaciousreasoning · 2022-10-18T02:46:33Z

components/brave_new_tab_ui/components/default/braveToday/customize/SourcesListEntry.tsx

    <Flex align='center' gap={8}>
      <FavIcon src={publisher.faviconUrl?.url} />
      <Text>{publisher.publisherName}</Text>
    </Flex>
-    <ToggleButton>
-      {subscribed ? Heart : HeartOutline}
+    <ToggleButton onClick={() => setFollowed(!followed)}>


Might be worth adding some padding/more gap to this button. It looks a bit odd with long names:

It might be because the Flex is collapsing the gap as things get too bit.

We can tidy this up when we add channel images, Leo's button and fallback images

fallaciousreasoning · 2022-10-18T02:47:38Z

components/brave_new_tab_ui/components/default/braveToday/customize/SourcesListEntry.tsx

-      {subscribed ? Heart : HeartOutline}
+    <ToggleButton onClick={() => setFollowed(!followed)}>
+      {followed
+        ? getLocale('braveNewsFollowButtonFollowing')


I don't know if this button is ever in a state where the source is not being followed? Non-followed sources are not shown in the list.

ok, I removed the other state

fallaciousreasoning · 2022-10-18T02:47:47Z

components/brave_new_tab_ui/components/default/braveToday/customize/SourcesListEntry.tsx

-      {subscribed ? Heart : HeartOutline}
+    <ToggleButton onClick={() => setSubscribed(!subscribed)}>
+      {subscribed
+        ? getLocale('braveNewsFollowButtonFollowing')


…ontent. follow / unfollow button text instead of heart icon

brave-builds · 2022-10-18T04:05:50Z

A Storybook has been deployed to preview UI for the latest push

petemill requested a review from fallaciousreasoning October 18, 2022 02:29

petemill self-assigned this Oct 18, 2022

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Oct 18, 2022

fallaciousreasoning approved these changes Oct 18, 2022

View reviewed changes

Desktop News customize: unfollow removes follow, and does not block c…

e13905d

…ontent. follow / unfollow button text instead of heart icon

petemill force-pushed the news-follow-button-unfollows branch from a1f246e to e13905d Compare October 18, 2022 03:20

petemill merged commit 09f5c90 into master Oct 18, 2022

petemill deleted the news-follow-button-unfollows branch October 18, 2022 06:21

petemill added this to the 1.46.x - Nightly milestone Oct 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop News customize: unfollow removes follow #15499

Desktop News customize: unfollow removes follow #15499

petemill commented Oct 18, 2022 •

edited

Loading

fallaciousreasoning left a comment

fallaciousreasoning Oct 18, 2022

petemill Oct 18, 2022

fallaciousreasoning Oct 18, 2022

fallaciousreasoning Oct 18, 2022

petemill Oct 18, 2022

petemill Oct 18, 2022

fallaciousreasoning Oct 18, 2022

petemill Oct 18, 2022

fallaciousreasoning Oct 18, 2022

fallaciousreasoning Oct 18, 2022

petemill Oct 18, 2022

fallaciousreasoning Oct 18, 2022

petemill Oct 18, 2022

fallaciousreasoning Oct 18, 2022

petemill Oct 18, 2022

fallaciousreasoning Oct 18, 2022

petemill Oct 18, 2022

fallaciousreasoning Oct 18, 2022

petemill Oct 18, 2022

brave-builds commented Oct 18, 2022

Desktop News customize: unfollow removes follow #15499

Desktop News customize: unfollow removes follow #15499

Conversation

petemill commented Oct 18, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

fallaciousreasoning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Oct 18, 2022

petemill commented Oct 18, 2022 •

edited

Loading