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.

Sign up for GitHub

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

Fabri QA for frames & deeplinks #1098

Merged
merged 3 commits into from
Oct 25, 2024
Merged

Fabri QA for frames & deeplinks #1098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b09405
Fabri QA for frames & deeplinks
nmalzieu Oct 25, 2024
04f11ba
also allow ' & " at beginning
nmalzieu Oct 25, 2024
c55753b
comments
nmalzieu Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions components/Chat/ChatPlaceholder/GroupChatPlaceholder.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { useGroupName } from "@hooks/useGroupName";
import { translate } from "@i18n";
import { useGroupQuery } from "@queries/useGroupQuery";
import { textPrimaryColor } from "@styles/colors";
import { isGroupTopic } from "@utils/groupUtils/groupId";
import { useCallback, useMemo } from "react";
import {
Keyboard,
Expand All @@ -25,6 +26,7 @@ type Props = {
};

export function GroupChatPlaceholder({ messagesCount }: Props) {
const topic = useConversationContext("topic");
const conversation = useConversationContext("conversation");
const onReadyToFocus = useConversationContext("onReadyToFocus");

Expand Down Expand Up @@ -74,9 +76,13 @@ export function GroupChatPlaceholder({ messagesCount }: Props) {
<View onLayout={onLayout} style={styles.chatPlaceholder}>
{!conversation && (
<View>
<ActivityIndicator style={styles.activitySpinner} />
{!topic && <ActivityIndicator style={{ marginBottom: 20 }} />}
<Text style={styles.chatPlaceholderText}>
{translate("opening_conversation")}
{topic
? isGroupTopic(topic)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should always be a group topic as it's in the GroupChatPlaceholder

Copy link
Contributor Author

@nmalzieu nmalzieu Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@alexrisch The check to display ChatPlaceholder or GroupChatPlaceholder is currently on conversation?.isGroup so it only works if the conversation was found. Do you wanna change it to isGroupTopic(conversationTopic) ?

? translate("group_not_found")
: translate("conversation_not_found")
: translate("opening_conversation")}
Comment on lines +81 to +85
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider extracting placeholder text logic for better readability

While the logic correctly implements the error message requirements, the nested ternary operators could be simplified for better maintainability.

+            const getPlaceholderText = () => {
+              if (!topic) return translate("opening_conversation");
+              return isGroupTopic(topic)
+                ? translate("group_not_found")
+                : translate("conversation_not_found");
+            };
             <Text style={styles.chatPlaceholderText}>
-              {topic
-                ? isGroupTopic(topic)
-                  ? translate("group_not_found")
-                  : translate("conversation_not_found")
-                : translate("opening_conversation")}
+              {getPlaceholderText()}
             </Text>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{topic
? isGroupTopic(topic)
? translate("group_not_found")
: translate("conversation_not_found")
: translate("opening_conversation")}
const getPlaceholderText = () => {
if (!topic) return translate("opening_conversation");
return isGroupTopic(topic)
? translate("group_not_found")
: translate("conversation_not_found");
};
<Text style={styles.chatPlaceholderText}>
{getPlaceholderText()}
</Text>

</Text>
</View>
)}
Expand Down
5 changes: 4 additions & 1 deletion screens/Conversation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,8 @@ const Conversation = ({
}
} else if (
route.params?.mainConversationWithPeer &&
!openedMainConvo.current
!openedMainConvo.current &&
!conversationTopicRef.current
) {
openedMainConvo.current = true;
openMainConversationWithPeer(
Expand Down Expand Up @@ -286,6 +287,7 @@ const Conversation = ({

const conversationContextValue = useMemo(
() => ({
topic: conversationTopic,
conversation,
messageToPrefill,
inputRef: textInputRef,
Expand All @@ -300,6 +302,7 @@ const Conversation = ({
tagsFetchedOnceForMessage,
}),
[
conversationTopic,
conversation,
messageToPrefill,
textInputRef,
Expand Down
4 changes: 2 additions & 2 deletions utils/regex.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const BOUNDARY_START_LOOKBEHIND = /(\s|\()/.source; // Either a space or a (
const BOUNDARY_START_LOOKBEHIND = /(\s|\(|"|')/.source; // Either a space or a (, or a " or a '
const BOUNDARY_START = new RegExp(`(?<=${BOUNDARY_START_LOOKBEHIND})|^`).source; // It must be start of the line or be preceded by lookbehind
const BOUNDARY_END_LOOKAHEAD = /(\s|\)|$|\.|!|\?|\r\n|\r|\n)/.source; // Either a space, the end of the text, or a ), a ., a !, a ?, a line break
const BOUNDARY_END_LOOKAHEAD = /(\s|\)|$|\.|!|\?|\r\n|\r|\n|"|')/.source; // Either a space, the end of the text, or a ), a ., a !, a ?, a line break, or a " or a '
const BOUNDARY_END = new RegExp(`(?=${BOUNDARY_END_LOOKAHEAD})`).source;
const WORD_CONTENT = /[^()/\s]/.source; // Not a space, not a ( or ), not a /

Expand Down
Loading