-
Notifications
You must be signed in to change notification settings - Fork 75
fix(newsletter): profile subscriber row layout and first-class Email digest action #3523
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,2 @@ | ||
| export { default as NewsletterSenderInfo } from './newsletterSenderInfo'; | ||
| export { NEWSLETTER_SENDER_INFO_HEIGHT } from './newsletterSenderInfo'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import React from 'react'; | ||
| import React, { useEffect } from 'react'; | ||
| import { Text, TouchableOpacity, View } from 'react-native'; | ||
| import { useIntl } from 'react-intl'; | ||
| import { useNavigation } from '@react-navigation/native'; | ||
|
|
@@ -11,8 +11,18 @@ import { useAppDispatch, useAuth } from '../../hooks'; | |
| import { toastNotification } from '../../redux/actions/uiAction'; | ||
| import { IconButton } from '../iconButton'; | ||
|
|
||
| /** | ||
| * Fixed row height, exported so ProfileSummaryView can report it through the | ||
| * summary card's moreHeight channel: the CollapsibleCard measures its content | ||
| * ONCE, so anything that appears after a query resolves must announce the | ||
| * space it takes or it gets clipped (vision-mobile#3522). | ||
| */ | ||
| export const NEWSLETTER_SENDER_INFO_HEIGHT = 28; | ||
|
|
||
| interface Props { | ||
| username: string; | ||
| /** Reports whether the row occupies space; stable identity expected. */ | ||
| onVisibilityChange?: (visible: boolean) => void; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -22,7 +32,7 @@ interface Props { | |
| * view is gated to the list owner server-side, so this mounts only on the | ||
| * own profile and stays silent while the lookup is unresolved or refused. | ||
| */ | ||
| const NewsletterSenderInfo = ({ username }: Props) => { | ||
| const NewsletterSenderInfo = ({ username, onVisibilityChange }: Props) => { | ||
| const intl = useIntl(); | ||
| const dispatch = useAppDispatch(); | ||
| const navigation = useNavigation(); | ||
|
|
@@ -33,6 +43,15 @@ const NewsletterSenderInfo = ({ username }: Props) => { | |
| ); | ||
|
|
||
| const subscribers = senderQuery.data?.subscribers; | ||
| const visible = !!subscribers; | ||
|
|
||
| useEffect(() => { | ||
| onVisibilityChange?.(visible); | ||
| return () => { | ||
| onVisibilityChange?.(false); | ||
| }; | ||
| }, [visible, onVisibilityChange]); | ||
|
|
||
| if (!subscribers) { | ||
| return null; | ||
| } | ||
|
|
@@ -44,7 +63,7 @@ const NewsletterSenderInfo = ({ username }: Props) => { | |
|
|
||
| return ( | ||
| <View style={styles.row}> | ||
| <Text style={styles.countText}> | ||
| <Text style={styles.countText} numberOfLines={1}> | ||
| {intl.formatMessage( | ||
| { id: 'newsletter.subscriber_count' }, | ||
| { weekly: subscribers.weekly ?? 0, monthly: subscribers.monthly ?? 0 }, | ||
|
|
@@ -66,13 +85,14 @@ const NewsletterSenderInfo = ({ username }: Props) => { | |
|
|
||
| const styles = EStyleSheet.create({ | ||
| row: { | ||
| height: NEWSLETTER_SENDER_INFO_HEIGHT, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With accessibility font scaling enabled, the subscriber count and Manage label can exceed this fixed 28-point height (and the existing Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in c45ed63: the fixed height is gone. As a sibling outside the measured card the row takes its natural height, so scaled text and the 30pt icon simply make it taller instead of clipping. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. Row shorter than icon NEWSLETTER_SENDER_INFO_HEIGHT fixes the row at 28px, but its IconButton child is fixed at 30×30px. The row therefore does not fully accommodate its own content and can still clip or overlap vertically. Agent Prompt
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in c45ed63: no fixed row height any more; the row sits outside the measured card in normal flow and its natural height contains the 30pt icon.
qodo-code-review[bot] marked this conversation as resolved.
Outdated
|
||
| flexDirection: 'row', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| paddingHorizontal: 16, | ||
| paddingTop: 6, | ||
| }, | ||
| countText: { | ||
| flexShrink: 1, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2. manage width remains unconstrained The PR makes the count text shrink, but the Manage action still has no fixed-width container or width constraint. A long localized action label can therefore consume or exceed the available row width on small devices, contrary to the required fixed action area. Agent Prompt
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in c45ed63: the Manage label is single line with maxWidth 120, and the count text keeps flexShrink with one line, so a long localized action cannot push the row content off narrow screens. |
||
| fontSize: 13, | ||
| color: '$primaryDarkGray', | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ import { makeCountFriendly } from '../../../utils/formatter'; | |
| import styles from './profileSummaryStyles'; | ||
| import getWindowDimensions from '../../../utils/getWindowDimensions'; | ||
| import { SheetNames } from '../../../navigation/sheets'; | ||
| import { NewsletterSenderInfo } from '../../newsletterSenderInfo'; | ||
| import { NEWSLETTER_SENDER_INFO_HEIGHT, NewsletterSenderInfo } from '../../newsletterSenderInfo'; | ||
|
|
||
| const DEVICE_WIDTH = getWindowDimensions().width; | ||
|
|
||
|
|
@@ -33,9 +33,30 @@ class ProfileSummaryView extends PureComponent<any, any> { | |
| super(props); | ||
| this.state = { | ||
| isShowPercentText: props.isShowPercentText, | ||
| senderInfoVisible: false, | ||
| }; | ||
| } | ||
|
|
||
| // The summary card's moreHeight channel is single-valued, so every consumer | ||
| // of extra height reports through this ONE combiner: the VP/RC bars toggle | ||
| // and the subscriber row would otherwise overwrite each other's height and | ||
| // the card would clip whichever reported first (vision-mobile#3522). | ||
| _reportMoreHeight = () => { | ||
| const { handleUIChange } = this.props; | ||
| const { isShowPercentText, senderInfoVisible } = this.state; | ||
| if (handleUIChange) { | ||
| handleUIChange( | ||
| (isShowPercentText ? 30 : 0) + (senderInfoVisible ? NEWSLETTER_SENDER_INFO_HEIGHT : 0), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the own profile is popped and reopened while the sender query remains in the in-memory cache, Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in c45ed63 by removing the arithmetic entirely: the row moved OUT of the CollapsibleCard and renders as a sibling below it in normal layout flow, so nothing is ever added to moreHeight and a cached first render cannot be counted twice. It follows isSummaryOpen, so collapsing the summary hides it too. |
||
| ); | ||
|
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
Outdated
|
||
| } | ||
| }; | ||
|
|
||
| _handleSenderInfoVisibility = (visible: boolean) => { | ||
| if (this.state.senderInfoVisible !== visible) { | ||
| this.setState({ senderInfoVisible: visible }, this._reportMoreHeight); | ||
| } | ||
| }; | ||
|
|
||
| _handleOnPressLink = (url: any) => { | ||
| if (url) { | ||
| Linking.openURL(url); | ||
|
|
@@ -277,7 +298,7 @@ class ProfileSummaryView extends PureComponent<any, any> { | |
|
|
||
| _renderBars = () => { | ||
| const { isShowPercentText } = this.state; | ||
| const { handleUIChange, hoursRC, hoursVP, isDarkTheme, percentRC, percentVP } = this.props; | ||
| const { hoursRC, hoursVP, isDarkTheme, percentRC, percentVP } = this.props; | ||
|
|
||
| const votingPowerHoursText = hoursVP && `• Full in ${hoursVP} hours`; | ||
| const votingPowerText = `Voting power: ${percentVP}% ${votingPowerHoursText || ''}`; | ||
|
|
@@ -288,9 +309,7 @@ class ProfileSummaryView extends PureComponent<any, any> { | |
| <TouchableOpacity | ||
| style={styles.barsContainer} | ||
| onPress={() => | ||
| this.setState({ isShowPercentText: !isShowPercentText }, () => { | ||
| handleUIChange(!isShowPercentText ? 30 : 0); | ||
| }) | ||
| this.setState({ isShowPercentText: !isShowPercentText }, this._reportMoreHeight) | ||
| } | ||
| > | ||
| <PercentBar | ||
|
|
@@ -323,7 +342,12 @@ class ProfileSummaryView extends PureComponent<any, any> { | |
| {this._renderIdentity()} | ||
| {this._renderMetadata()} | ||
| {this._renderFollowerStats()} | ||
| {!!isOwnProfile && !!username && <NewsletterSenderInfo username={username} />} | ||
| {!!isOwnProfile && !!username && ( | ||
| <NewsletterSenderInfo | ||
| username={username} | ||
| onVisibilityChange={this._handleSenderInfoVisibility} | ||
| /> | ||
| )} | ||
| {this._renderBars()} | ||
| </Fragment> | ||
| ); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.