Add delete button to Lightbox #41

jonathan-waarneming-nl · 2024-05-02T16:14:51Z

Displays a delete button in Lightbox. If there is also content, the button is displayed below this content.

Part of: https://github.com/observation/app/issues/349

jonathan-waarneming-nl · 2024-05-02T17:18:41Z

src/components/__tests__/Lightbox.test.tsx

Zoals ook gezegd in de andere PR voor dit issue: ik weet niet hoe ik deze test moet implementeren:
"Swiping to the left and pressing the delete button, will delete the second photo".

Dit omdat ik niet goed inzie hoe het swipen kan gesimuleerd worden.

Een swipe doen kan zo te zien niet via testing library, maar second best is om een onImageIndexChange call te triggeren vanuit ImageView. Zou dat kunnen?

Je kunt zo'n onImageIndexChange triggeren als je met een testID erbij kan, maar dat was nu niet 't geval.
Wat ik nu heb gedaan is een mock met requireActual, die de mock inject. Dus 't is uiteindelijk gelukt.

jonathan-waarneming-nl · 2024-05-02T17:21:14Z

src/components/Lightbox.tsx

@@ -52,6 +60,15 @@ const getLightboxFooterComponent =
        )}
        {content && <View style={styles.footerItem}>{content}</View>}
      </View>
+      {onPressDelete && (


@SjaakSchilperoort we hadden het eerst over om de button(s) om de mogelijk aanwezige content te plaatsen, maar ik zag dat de buttons in het design een redelijk grote margin hebben en dus heb ik dit maar onderaan gezet.
Een voordeel is wel dat er geen snapshot changes zijn voor componenten die de onPressDelete niet meegeven.

SjaakSchilperoort

Paar opmerkingen.

Terzijde: ik zie drie commits met de beschrijving "WIP". Kun je de commits waar een PR uit bestaat begrijpelijk maken door altijd te beschrijven wat het doel van een commit is? Ik doe het zelf ook altijd. Het is dan tijdens een code review gemakkelijker om te achterhalen waar een bepaalde code-wijziging voor nodig is. Als je enkel wat vergeten bent in een vorige commit kun je natuurlijk ook commits samenvoegen. Of als dat voor de begrijpelijkheid beter is, kun je ook de volgorde van commits aanpassen. Kost weinig moeite, maar is voor een reviewer erg fijn.

SjaakSchilperoort · 2024-05-03T07:13:38Z

src/components/__tests__/Lightbox.test.tsx

Een swipe doen kan zo te zien niet via testing library, maar second best is om een onImageIndexChange call te triggeren vanuit ImageView. Zou dat kunnen?

SjaakSchilperoort · 2024-05-03T12:28:18Z

src/components/Lightbox.tsx

+  const [imageIndexAfterSwipe, setImageIndexAfterSwipe] = useState<number>()
+
+  const imageIndex = index ?? 0


Ik zou swipe niet in de naam van deze velden gebruiken. Dit is te specifiek en swipe-support is ook niet de verantwoordelijkheid van deze component maar van ImageView. Hier een suggestie:

Suggested change

const [imageIndexAfterSwipe, setImageIndexAfterSwipe] = useState<number>()

const imageIndex = index ?? 0

const [currentImageIndex, setCurrentImageIndex] = useState<number>()

const initialImageIndex = index ?? 0

Ja, die is wel elegant

SjaakSchilperoort · 2024-05-03T12:30:06Z

src/components/Lightbox.tsx

+
+  const imageIndex = index ?? 0
+
+  const onPressDelete = onDelete ? () => onDelete(imageIndexAfterSwipe ?? imageIndex) : undefined


Zou de default imageIndex naar de state kunnen? Dus:

const initialImageIndex = index ?? 0 const [currentImageIndex, setCurrentImageIndex] = useState<number>(initialImageIndex)

Dan wordt het hier simpeler:

const onPressDelete = onDelete ? () => onDelete(currentImageIndex) : undefined

Nee.
Het component is altijd aanwezig. Dus de state die je aanmaakt met useState is altijd hetzelfde object.

Als je 3 foto's hebt en je opent de eerste en slide naar de laatste, is de waarde van currentImageIndex 2.
Je sluit de Lightbox en opent weer de eerste. Op dit moment is de waarde currentImageIndex nog steeds 2 (ipv 0).

Ik dacht dat mijn methode dit afving, maar ik zie dat ik hetzelfde probleem heb...

Wat wel zou kunnen werken is alleen de Lightbox tonen als er een foto is geselecteerd. Dan kunnen we jouw methode gebruiken en is 't wat makkelijker. Dus in de Observation app:

{photoIndex && <Lightbox index={photoIndex} onClose={() => setPhotoIndex(undefined)} photos={uris} onDelete={onDelete} /> }

Wat vind je hiervan?

jonathan-waarneming-nl · 2024-05-03T13:17:29Z

Kun je de commits waar een PR uit bestaat begrijpelijk maken door altijd te beschrijven wat het doel van een commit is?

Ik begrijp 't doel, maar nieuwe code in de RN components is niet bepaald fijn om te maken. Je moet een commit maken, pushen, ophalen in de andere repo, en dan kom je erachter dat 't niet klopt. Dus na een tijdje ben je 't wel zat. Dus als er "WIP" staat (wat ik volgens mij zelden doe in andere branches) moet je lezen als "Weer fout, nu dan?".

SjaakSchilperoort · 2024-05-03T14:40:51Z

Kun je de commits waar een PR uit bestaat begrijpelijk maken door altijd te beschrijven wat het doel van een commit is?

Ik begrijp 't doel, maar nieuwe code in de RN components is niet bepaald fijn om te maken. Je moet een commit maken, pushen, ophalen in de andere repo, en dan kom je erachter dat 't niet klopt. Dus na een tijdje ben je 't wel zat. Dus als er "WIP" staat (wat ik volgens mij zelden doe in andere branches) moet je lezen als "Weer fout, nu dan?".

Die werkwijze levert dus rommel op in de commits, maar dat kun je toch opruimen voordat je een review opent?

Ik dacht dat je in package.json ook naar een directory op je file system kon verwijzen. Dan kun je wijzigingen in de componenten uitproberen zonder de components-lib steeds te moeten pushen. Als dat niet goed werkt dan kun je ook de component naar de observation app kopiëren, daar ontwikkelen, en als alles werkt de nieuwe versie weer terug te kopiëren.

jonathan-waarneming-nl · 2024-05-06T08:54:43Z

Die werkwijze levert dus rommel op in de commits, maar dat kun je toch opruimen voordat je een review opent?

Ik dacht niet dat dit erg was, maar zal er voortaan aan denken.

Ik dacht dat je in package.json ook naar een directory op je file system kon verwijzen.

Dit zal 't leven veel gemakkelijker maken, zal eens kijken

…nothing

SjaakSchilperoort · 2024-05-06T11:03:14Z

src/components/Lightbox.tsx

-  />
-)
+const Lightbox = ({ index, onClose, onDelete, photos, title, description, content, style }: Props) => {
+  const initialIndex = index ?? 0


Of dit om consistent te blijven?

Suggested change

const initialIndex = index ?? 0

const initialImageIndex = index ?? 0

SjaakSchilperoort · 2024-05-06T11:11:33Z

src/components/Lightbox.tsx

+    marginHorizontal: theme.margin.common,
+  },
+  buttonContainer: {
+    width: '50%',


Dit gaat mis als we drie buttons hebben. Ik denk dat het beter is om dit te vervangen door flex: 0.5, dat gaat wel goed bij >2 buttons.

SjaakSchilperoort

👌🏻

anoukdriessen

Wat mij betreft prima zo 👍

jonathan-waarneming-nl commented May 2, 2024

View reviewed changes

jonathan-waarneming-nl requested review from SjaakSchilperoort and anoukdriessen May 2, 2024 17:18

jonathan-waarneming-nl commented May 2, 2024

View reviewed changes

SjaakSchilperoort reviewed May 3, 2024

View reviewed changes

Jonathan added 11 commits May 6, 2024 11:15

Add delete button

e422833

WIP

be35706

WIP

6b3163d

WIP

e522d82

Add unit tests

168c9b4

Move button to the bottom

c343b9a

Use new design

6d94164

Add hit slop

119bd46

Update to new margin

0eb9ab4

Add Unit test + fix clicking in the middle of the delete button does …

357c071

…nothing

Update version

83e699c

jonathan-waarneming-nl force-pushed the feature/add-delete-button branch from eba0817 to 83e699c Compare May 6, 2024 09:16

Jonathan added 3 commits May 6, 2024 11:51

Use better variable names

768ce68

Use new margin

0ff2a3e

Use initial index for imageIndex to avoid remounts

a57afda

SjaakSchilperoort reviewed May 6, 2024

View reviewed changes

Use flex: 0.5 for every action + use consistent variable naming

a3713a6

SjaakSchilperoort approved these changes May 6, 2024

View reviewed changes

anoukdriessen approved these changes May 6, 2024

View reviewed changes

jonathan-waarneming-nl merged commit 54e9a75 into develop May 6, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add delete button to Lightbox #41

Add delete button to Lightbox #41

jonathan-waarneming-nl commented May 2, 2024 •

edited

Loading

jonathan-waarneming-nl May 2, 2024

SjaakSchilperoort May 3, 2024

jonathan-waarneming-nl May 3, 2024

jonathan-waarneming-nl May 2, 2024 •

edited

Loading

SjaakSchilperoort left a comment

SjaakSchilperoort May 3, 2024

SjaakSchilperoort May 3, 2024

jonathan-waarneming-nl May 3, 2024

SjaakSchilperoort May 3, 2024

jonathan-waarneming-nl May 3, 2024

jonathan-waarneming-nl commented May 3, 2024

SjaakSchilperoort commented May 3, 2024

jonathan-waarneming-nl commented May 6, 2024

SjaakSchilperoort May 6, 2024

SjaakSchilperoort May 6, 2024

SjaakSchilperoort left a comment

anoukdriessen left a comment

		const [imageIndexAfterSwipe, setImageIndexAfterSwipe] = useState<number>()

		const imageIndex = index ?? 0


		const imageIndex = index ?? 0

		const onPressDelete = onDelete ? () => onDelete(imageIndexAfterSwipe ?? imageIndex) : undefined

	const initialIndex = index ?? 0
	const initialImageIndex = index ?? 0

Add delete button to Lightbox #41

Add delete button to Lightbox #41

Conversation

jonathan-waarneming-nl commented May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-waarneming-nl May 2, 2024 • edited Loading

Choose a reason for hiding this comment

SjaakSchilperoort 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

jonathan-waarneming-nl commented May 3, 2024

SjaakSchilperoort commented May 3, 2024

jonathan-waarneming-nl commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SjaakSchilperoort left a comment

Choose a reason for hiding this comment

anoukdriessen left a comment

Choose a reason for hiding this comment

jonathan-waarneming-nl commented May 2, 2024 •

edited

Loading

jonathan-waarneming-nl May 2, 2024 •

edited

Loading