Skip to content
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

fix: inline display for comment links #17496

Merged
merged 14 commits into from
Apr 28, 2023
Merged
3 changes: 2 additions & 1 deletion src/components/AnchorForAttachmentsOnly/index.native.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import React from 'react';
import * as anchorForAttachmentsOnlyPropTypes from './anchorForAttachmentsOnlyPropTypes';
import BaseAnchorForAttachmentsOnly from './BaseAnchorForAttachmentsOnly';
import * as StyleUtils from '../../styles/StyleUtils';
import styles from '../../styles/styles';

// eslint-disable-next-line react/jsx-props-no-spreading
const AnchorForAttachmentsOnly = props => <BaseAnchorForAttachmentsOnly {...props} style={styles.mw100} />;
const AnchorForAttachmentsOnly = props => <BaseAnchorForAttachmentsOnly {...props} style={[...StyleUtils.parseStyleAsArray(props.style), styles.mw100]} />;

AnchorForAttachmentsOnly.propTypes = anchorForAttachmentsOnlyPropTypes.propTypes;
AnchorForAttachmentsOnly.defaultProps = anchorForAttachmentsOnlyPropTypes.defaultProps;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import htmlRenderers from './HTMLRenderers';
import * as HTMLEngineUtils from './htmlEngineUtils';
import styles from '../../styles/styles';
import fontFamily from '../../styles/fontFamily';
import defaultViewProps from './defaultViewProps';

const propTypes = {
/** Whether text elements should be selectable */
Expand Down Expand Up @@ -50,8 +51,6 @@ const customHTMLElementModels = {
}),
};

const defaultViewProps = {style: [styles.alignItemsStart, styles.userSelectText]};

// We are using the explicit composite architecture for performance gains.
// Configuration for RenderHTML is handled in a top-level component providing
// context to RenderHTMLSource components. See https://git.io/JRcZb
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ const AnchorRenderer = (props) => {
if (isAttachment) {
return (
<AnchorForAttachmentsOnly
style={styles.alignItemsStart}
source={tryResolveUrlFromApiRoot(attrHref)}
displayName={displayName}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ const ImageRenderer = (props) => {
>
{({show}) => (
<PressableWithoutFocus
style={styles.noOutline}
styles={[styles.noOutline, styles.alignItemsStart]}
onPress={show}
onLongPress={event => showContextMenuForReport(event, anchor, reportID, action, checkIfContextMenuActive)}
>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import styles from '../../../styles/styles';

export default {
style: [styles.dFlex, styles.userSelectText],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, @aswin-s Do you mind explaining to me why we had to remove the styles.alignItemsStart from Native platforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alignItemsStart was applied to the root renderer for both web and native platforms. While fixing it for web platform I removed it for both platforms and moved it to inner renderers which are responsible for rendering individual components. You can checkout the changes for Image and Attachment renders. This style is needed for both native and web platforms but applying it at root node has unintended effects for web.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the change was needed only for web platforms right, and switch back only for native should make no harm? I came across the problem with dFlex on native, as with it applied on root render component, rendered text component span on whole line, and applying it to the renderer itself does nothing as it is intended to be inline rather than block

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to keep the same styling layout across platforms as much as possible. This will reduce the complexity. For now, we have a simple solution that works so let's go with that.

};
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import styles from '../../../styles/styles';

export default {
// For web platform default to block display. Using flex on root view will force all
// child elements to be block elements even when they have display inline added to them.
// This will affect elements like <a> which are inline by default.
style: [styles.dBlock, styles.userSelectText],
aswin-s marked this conversation as resolved.
Show resolved Hide resolved
};

3 changes: 3 additions & 0 deletions src/styles/utilities/display.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,7 @@ export default {
dInline: {
display: 'inline',
},
dBlock: {
display: 'block',
},
};