[#212] Remove extra white-space in PersonListCard

[baskargopinath]

Fixes #212

Reduced spacing between the index and name to remove the extra white-space by adjusting spacing of Hbox

Before:

After:

[canihasreview]

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

[canihasreview]

v1

@baskargopinath submitted v1 for review.

• [v1 1/1] Adjust spacing

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/213/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

[damithc]

Thanks for the PR, @baskargopinath
Can you update the commit message to be more detailed, as per the convention given in https://se-education.org/guides/conventions/git.html ?
You can also check a few previous commits to see the level of details AB3 commits usually have.
The commit message can also explain how 5 -> 0.5 results in a UI change of double-space to single-space (as that is not immediately obvious).

[baskargopinath]

okay prof @damithc i have updated the commit msg based on the specification

[damithc]

@baskargopinath

1. After making a change, use CanIHasReview to post the new version (similar to this)
2. There seems to an unrelated commit in this PR now

[canihasreview]

v2

@baskargopinath submitted v2 for review.

• [v2 1/1] Adjust spacing

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/213/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

[damithc]

Good progress @baskargopinath
The commit message is better now. The subject can be more informative though.
Also, added a query about the change.

[damithc]

I'm still curious how changing 5 -> 0.5 results in halving of the spacing.

[baskargopinath]

@damithc

The spacing attribute in the HBox only affects the horizontal space between the direct child elements of the HBox, not the spacing within the content of those child elements. In your case, the HBox contains two Label elements: one for the ID and one for the name. The spacing attribute adjusts the space between these two Label elements, not the space between characters within the text of each Label.

1. HBox Spacing: The spacing="0.5" in the HBox affects the space between its direct children, which are the Label elements with fx:id="id" and fx:id="name". This means it reduces the space between the ID and the name labels.

2. Text Content: The spacing within the text content of a Label (such as spaces between words or characters) is not affected by the HBox’s spacing attribute. This spacing is managed by the text rendering engine and the font metrics, which are not controlled by the HBox.

3. Effect on Layout:

   • Between Labels: The reduced spacing value (0.5) decreases the space between the two Label elements, making them appear closer together.
   • Within Labels: The spacing between words or characters within the text of each Label remains unchanged because it’s controlled by the font and text properties.

In summary, the spacing attribute in the HBox controls the space between its child elements (the Labels), not the space within the text content of those child elements. This is why changing the spacing value only affects the distance between the ID and name labels, not the spacing within the words in the labels.

[baskargopinath]

i think this is why the spacing between words is fine but the issue was only between the index and name

(Index) [SPACE] (name)

Also the changing of the value to 0.5 is not by some exact form of calculation or formula,

it was more through reducing the value and seeing the reflected change in the UI until the reduction resulted in what looked like a halving of the space

[baskargopinath]

i was thinking if i should add this to the commit message body but i didnt want it to be too long

[damithc]

i was thinking if i should add this to the commit message body but i didnt want it to be too long

You can mention it briefly i.e., you are adjusting the spacing attribute, which determines the spacing between child elements, and the value 0.5 seems to produce the desired gap.

[baskargopinath]

okay will do it @damithc

[canihasreview]

v3

@baskargopinath submitted v3 for review.

• [v3 1/1] Fix double-whitespace by reducing HBox spacing

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/213/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

[baskargopinath]

@damithc one thing i want to ask, this change will result in the ui change so we need to update ReadMe.md but should that be done in the same PR or we should create a seperate issue and PR for that

[damithc]

@damithc one thing i want to ask, this change will result in the ui change so we need to update ReadMe.md but should that be done in the same PR or we should create a seperate issue and PR for that

Yes, in the same commit. Otherwise the PR (and the commit) takes the code into an incorrect state.
That said, the current screenshot can remain if it is good enough for the purpose. We don't need to update screenshots every time we change the UI in a minor way. Most likely the keeping the original screenshot will not hinder the reader in any way.

[baskargopinath]

@damithc one thing i want to ask, this change will result in the ui change so we need to update ReadMe.md but should that be done in the same PR or we should create a seperate issue and PR for that

Yes, in the same commit. Otherwise the PR (and the commit) takes the code into an incorrect state. That said, the current screenshot can remain if it is good enough for the purpose. We don't need to update screenshots every time we change the UI in a minor way. Most likely the keeping the original screenshot will not hinder the reader in any way.

okay makes sense

[damithc]

Merged, with some minor tweaks to the commit message.
Thanks for the PR, @baskargopinath