Skip to content

Conversation

@algorythmice
Copy link
Contributor

@algorythmice algorythmice commented Jan 31, 2026

Contribution

Règles de contribution

Caution

Afin de garantir une application stable et pérenne dans le temps, nous t'invitons à vérifier que tu as bien respecté les règles de contribution. Sans cela, ta Pull Request ne pourra pas être examinée.

  • Cette Pull Request porte sur une seule fonctionnalité ou un seul correctif.
  • Cette Pull Request n'est pas faite essentiellement avec de l'IA.
  • Pour tout changement majeur, j’ai créé une issue afin d’échanger avec les mainteneurs de Papillon sur la meilleure façon de l’intégrer.
  • Ma Pull Request respecte les conventions Conventional Commits et Conventional Branch ainsi que les conventions de codage de l'application.
  • J’ai testé mes modifications sur iOS et Android, et l’application fonctionne correctement.
  • J’emploie un langage informel, clair et concis dans mes messages.
  • J’ai documenté mes changements de manière appropriée, soit dans la description de la Pull Request, soit dans le GitBook.
  • J’ai ajouté les traductions nécessaires dans au moins un fichier de langue.

Résumé des changements

Cette PR remplace les anciennes listes natives d'Android par des listes dans le style de Papillon.

  • Ajout du composant ActionMenu
  • Remplacement des MenuView par des ActionMenu
    Le composant a été conçu pour être un remplacement direct des balises sans changer la structure de l'élément:
<MenuView>
</MenuView>

Capture(s) d'écran

Screenshot_20260131_005449_Papillon

Screenshot_20260131_005458_Papillon

Screenshot_20260131_005505_Papillon

Copilot AI review requested due to automatic review settings January 31, 2026 00:01
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a new ActionMenu component intended to replace Android’s native menu lists with Papillon-styled menus, and updates the Grades screens to use it instead of MenuView.

Changes:

  • Added ui/components/ActionMenu.tsx with an iOS MenuView passthrough and a custom Android modal menu implementation.
  • Replaced MenuView usage with ActionMenu in Grades header period selector and the Averages method selector.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File Description
ui/components/ActionMenu.tsx New cross-platform menu component; custom Android rendering and iOS passthrough.
app/(tabs)/grades/index.tsx Swaps period selector from MenuView to ActionMenu.
app/(tabs)/grades/atoms/Averages.tsx Swaps averages method selector from MenuView to ActionMenu and adds a missing action id.

const primaryColor = colors.primary;
const cardColor = colors.card;

const triggerRef = useRef<View>(null);
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

TypeScript strict mode is enabled (tsconfig.json sets strict: true), so useRef<View>(null) will be a type error because null isn’t assignable to View. Use a nullable ref type (e.g., useRef<View | null>(null)), and update the optional chaining accordingly if needed.

Suggested change
const triggerRef = useRef<View>(null);
const triggerRef = useRef<View | null>(null);

Copilot uses AI. Check for mistakes.
Comment on lines 151 to 155
return {
position: "absolute" as const,
top: position.y + position.height + 8,
left: Math.min(position.x, 100),
};
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

left: Math.min(position.x, 100) clamps the menu’s left position to at most 100px, so triggers placed to the right will always render the menu at x=100. This looks unintended and will misplace the menu on many layouts. Clamp left within the screen bounds instead (e.g., using Dimensions.get('window').width and the menu width/padding), or align based on the trigger center.

Copilot uses AI. Check for mistakes.
Comment on lines +124 to +128
function open() {
setVisible(true);
setTimeout(() => {
triggerRef.current?.measureInWindow((x, y, width, height) => {
setPosition({ x, y, width, height });
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

measureInWindow can be unreliable on Android if the referenced wrapper <View> is flattened out of the native hierarchy (RN collapsable optimization). Since the ref is attached to a wrapper with no styles, consider adding collapsable={false} (or a non-empty style) so measuring consistently returns correct coordinates.

Copilot uses AI. Check for mistakes.
} catch {}

interface MenuAction {
id: string;
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

id is required here, but existing MenuView usage in this repo sometimes omits id for grouping actions (e.g. actions that only contain subactions). If this component is meant to be a drop-in replacement, consider making id optional for non-selectable/group items and generating a stable key for rendering on Android.

Suggested change
id: string;
id?: string;

Copilot uses AI. Check for mistakes.
Comment on lines 183 to 186
<TouchableOpacity
onPress={() => setSubmenu(null)}
style={styles.header}
>
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

Submenu navigation only supports a single level: the “back” handler always does setSubmenu(null), so nested submenus can’t return to their parent submenu. If multi-level subactions are possible, track a stack of menus (or parent references) so back returns to the previous submenu.

Copilot uses AI. Check for mistakes.
Comment on lines 151 to 155
return {
position: "absolute" as const,
top: position.y + position.height + 8,
left: Math.min(position.x, 100),
};
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

Menu positioning only places the menu below the trigger (top: y + height + 8) without any screen-bound clamping. If the trigger is near the bottom of the screen, the menu will render off-screen. Consider clamping top (and potentially flipping above the trigger when there isn’t enough space) based on window height and measured menu height.

Copilot uses AI. Check for mistakes.
Comment on lines 24 to 28
image?: string;
imageColor?: string;
destructive?: boolean;
disabled?: boolean;
subactions?: MenuAction[];
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

Other MenuView call sites in this repo use attributes: { destructive, disabled }. The Android renderer here only reads action.destructive / action.disabled, so those existing actions won’t behave the same if migrated. Consider supporting the library’s attributes field and mapping it to these flags.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@Dev-LeChacal Dev-LeChacal left a comment

Choose a reason for hiding this comment

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

Magnifique !
LGTM

@guguss-31
Copy link

#581
Merci beaucoup ❤️ !

Copy link
Contributor

@Dev-LeChacal Dev-LeChacal left a comment

Choose a reason for hiding this comment

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

Utilise l'ActionMenu dans le component ChipButton qui utilise aussi le MenuView

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

Comment on lines +135 to +147
function open() {
setVisible(true);
setTimeout(() => {
triggerRef.current?.measureInWindow((x, y, width, height) => {
setPosition({ x, y, width, height });
});
}, 0);
}

function close() {
setVisible(false);
setSubmenuStack([]);
}
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

open() schedules a setTimeout that calls setPosition but it’s never cleared on unmount, and position is not reset on close(). This can cause setState-after-unmount warnings and stale positioning on the next open. Consider using requestAnimationFrame (or storing/clearing the timeout id in an effect cleanup) and resetting position to null when closing/opening.

Copilot uses AI. Check for mistakes.
Comment on lines 21 to 44
interface MenuAction {
id?: string;
title: string;
subtitle?: string;
state?: "on" | "off" | "mixed";
image?: string;
imageColor?: number | ColorValue;
destructive?: boolean;
disabled?: boolean;
subactions?: MenuAction[];
displayInline?: boolean;
attributes?: {
destructive?: boolean;
disabled?: boolean;
state?: "on" | "off" | "mixed";
};
}

interface ActionMenuProps {
actions: MenuAction[];
children: ReactNode;
onPressAction?: (event: { nativeEvent: { event: string } }) => void;
title?: string;
}
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

MenuAction is redefined locally but other call sites (e.g. ChipButton) still use MenuAction from @react-native-menu/menu. This duplication can drift and causes avoidable type incompatibilities. Consider exporting a single MenuAction type from ActionMenu (or re-exporting the library type) and using it consistently for actions across the app.

Copilot uses AI. Check for mistakes.
@@ -1,5 +1,5 @@
import { Papicons } from "@getpapillon/papicons";
import { MenuAction, MenuView } from '@react-native-menu/menu';
import { MenuAction } from '@react-native-menu/menu';
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

MenuAction is only used as a TypeScript type here. Use a type-only import (import type) to ensure it’s erased at runtime and to match existing project patterns (e.g. ui/components/AlertProvider.tsx).

Suggested change
import { MenuAction } from '@react-native-menu/menu';
import type { MenuAction } from '@react-native-menu/menu';

Copilot uses AI. Check for mistakes.
Comment on lines +416 to 418
<ActionMenu
onPressAction={({ nativeEvent }) => {
const actionId = nativeEvent.event;
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

PR description says MenuView usages are replaced by ActionMenu, but there are still multiple MenuView imports/usages in the repo (e.g. app/(features)/(news)/specific.tsx, app/(features)/attendance.tsx, app/(modals)/profile.tsx, app/(modals)/wallpaper.tsx, app/(tabs)/calendar/event/[id].tsx). Either update the description/scope, or complete the replacement in this PR/follow-up.

Copilot uses AI. Check for mistakes.
Comment on lines +190 to +198
<View
ref={triggerRef}
collapsable={false}
onTouchEnd={(e) => {
if (!visible) {
e.stopPropagation();
open();
}
}}
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

On Android, the menu is opened via the wrapper View’s onTouchEnd. This is likely to never fire when the child is a Touchable*/Pressable (e.g. TabHeaderTitle is a TouchableOpacity with a default onPress), which would prevent the menu from opening. Consider using responder capture (onStartShouldSetResponderCapture/onResponderRelease) or an explicit Pressable trigger wrapper so the ActionMenu reliably receives the gesture, and gate it behind actions.length > 0 to avoid intercepting taps when there’s no menu.

Copilot uses AI. Check for mistakes.
Copy link
Member

@raphckrman raphckrman left a comment

Choose a reason for hiding this comment

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

Hello, merci pour ta Pull Request qui vise à résoudre un problème important sur Android, j'ai fait quelques petits commentaires pour améliorer tout ça, il faudrait aussi faire attention aux safe-areas, par exemple sur un appareil de développement, le menu passe dans l'encoche de la caméra.

Comment on lines 16 to 19
let NativeMenuView: any = null;
try {
NativeMenuView = require("@react-native-menu/menu").MenuView;
} catch {}
Copy link
Member

Choose a reason for hiding this comment

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

Il faudrait peut-être ajouter une gestion des erreurs ici, et peut-être de charger MenuView uniquement si l'appareil est sous iOS puisqu'il est utilisé seulement dans ce cas?

} from "react-native";
import { useTheme } from "@react-navigation/native";

let NativeMenuView: any = null;
Copy link
Member

Choose a reason for hiding this comment

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

Il faudrait retirer le any pour le remplacer par un typage plus strict

Comment on lines 21 to 37
interface MenuAction {
id?: string;
title: string;
subtitle?: string;
state?: "on" | "off" | "mixed";
image?: string;
imageColor?: number | ColorValue;
destructive?: boolean;
disabled?: boolean;
subactions?: MenuAction[];
displayInline?: boolean;
attributes?: {
destructive?: boolean;
disabled?: boolean;
state?: "on" | "off" | "mixed";
};
}
Copy link
Member

Choose a reason for hiding this comment

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

C'est super le fait que ce soit les mêmes props entre la version native et ta version, mais pourquoi ne pas réutiliser les types de MenuView? Ça pourrait éviter de dupliquer du code et ça assure une cohérence à 100% avec la librairie native dans le cas où l'utilisateur est sous iOS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oui c'est vrai que c'est plus malin merci!

interface ActionMenuProps {
actions: MenuAction[];
children: ReactNode;
onPressAction?: (event: { nativeEvent: { event: string } }) => void;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
onPressAction?: (event: { nativeEvent: { event: string } }) => void;
onPressAction?: ({ nativeEvent }: NativeActionEvent) => void;

Comment on lines 39 to 44
interface ActionMenuProps {
actions: MenuAction[];
children: ReactNode;
onPressAction?: (event: { nativeEvent: { event: string } }) => void;
title?: string;
}
Copy link
Member

Choose a reason for hiding this comment

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

Ce serait aussi cool ici d'utiliser les types de la librairie native

style={styles.header}
>
<Text style={[styles.back, { color: textColor }]}>
‹ {currentSubmenu.title}
Copy link
Member

Choose a reason for hiding this comment

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

On retrouve le même problème avec ce symbole Unicode, une Papicons ne ferait pas mieux l'affaire?

Comment on lines 254 to 255
minWidth: 260,
maxWidth: 320,
Copy link
Member

Choose a reason for hiding this comment

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

Tu es sûr de ces valeurs ? Papillon est utilisé sur beaucoup d'appareils différents, ça me semble un peu risqué codé ça en dur

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oui j'y ai pensé mais je ne sais pas trop comment remplacer ça si tu a une idée

Copy link
Contributor Author

Choose a reason for hiding this comment

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

paddingVertical: 10,
paddingHorizontal: 14,
borderBottomWidth: StyleSheet.hairlineWidth,
borderBottomColor: "rgba(128,128,128,0.2)",
Copy link
Member

Choose a reason for hiding this comment

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

Il me semble que Papillon a une couleur précise pour les bordures, il faudrait peut-être voir pour l'utiliser, ça permet de modifier partout et rester dans le theme de Papillon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oui j'ai trouvé dans un autre composant: borderBottomColor: (Platform.OS === 'ios' && !runsIOS26) ? colors.border : undefined,

Comment on lines 276 to 277
flexDirection: "row",
alignItems: "center",
Copy link
Member

Choose a reason for hiding this comment

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

c'est typiquement ce que peut faire Stack, tu gagnerais en lisibilité etc en utilisant ce composant

borderRadius: 10,
},
itemSelected: {
backgroundColor: "rgba(0,102,204,0.12)",
Copy link
Member

Choose a reason for hiding this comment

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

Je suis pas méga fan de la couleur, et du fait qu'elle soit en dur dans le code, un avis sur ça @ecnivtwelve ou @tom-things ?

Dev-LeChacal

This comment was marked as spam.

Copy link
Member

@raphckrman raphckrman left a comment

Choose a reason for hiding this comment

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

Hello, y'a eu pas mal de changements, c'est possible d'avoir un screen de comment ça rend avec les Papicons? Tu as géré aussi pour pas les safe-areas ?

NativeMenuView = mod?.MenuView ?? null;
} catch (err: unknown) {
console.warn("ActionMenu: impossible de charger @react-native-menu/menu MenuView:", err);
NativeMenuView = null;
Copy link
Member

Choose a reason for hiding this comment

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

Tu l'as déjà mis en null juste avant

const mod = require("@react-native-menu/menu");
NativeMenuView = mod?.MenuView ?? null;
} catch (err: unknown) {
console.warn("ActionMenu: impossible de charger @react-native-menu/menu MenuView:", err);
Copy link
Member

Choose a reason for hiding this comment

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

On a une méthode dans le logger pour ça

}
}

type MenuAction = NativeMenuAction;
Copy link
Member

Choose a reason for hiding this comment

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

Pourquoi ne pas utiliser directement le type NativeMenuAction ?

Comment on lines 33 to 38
interface ActionMenuProps {
actions: MenuAction[];
children: ReactNode;
onPressAction?: ({ nativeEvent }: NativeActionEvent) => void;
title?: string;
}
Copy link
Member

Choose a reason for hiding this comment

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

Ce serait possible, dans la continuité de ce qui est déjà fait juste avant, de reprendre les props de la librairie native? Pour être sur que c'est le même fonctionnement

Comment on lines 62 to 66
const colorText = action.imageColor
? action.imageColor
: destructive
? destructiveColor
: textColor;
Copy link
Member

Choose a reason for hiding this comment

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

l'opération ternaire est un peu chaotique, y'en a deux imbriqué

@algorythmice
Copy link
Contributor Author

Hello, y'a eu pas mal de changements, c'est possible d'avoir un screen de comment ça rend avec les Papicons? Tu as géré aussi pour pas les safe-areas ?

J'ai pris en compte les safe-areas et sinon voila ce que ça rend avec les Papicons

Screenshot_20260205_220522_Papillon
Screenshot_20260205_220527_Papillon
Screenshot_20260205_220534_Papillon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants