se-edu / addressbook-level3

:ab::three: Address Book sample application (Level 3)
https://se-education.org/addressbook-level3
MIT License
27 stars 411 forks source link

[#212] Remove extra white-space in PersonListCard #213

Closed baskargopinath closed 1 month ago

baskargopinath commented 1 month ago

Fixes #212

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

Before:

image

After:

Screenshot 2024-06-05 at 4 48 49 PM
canihasreview[bot] commented 1 month ago

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

See this repository's contribution guide for more information.

canihasreview[bot] commented 1 month ago

v1

@baskargopinath submitted v1 for review.

(:books: Archive)

Checkout this PR version locally ```sh 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.
baskargopinath commented 1 month ago

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

damithc commented 1 month ago

@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[bot] commented 1 month ago

v2

@baskargopinath submitted v2 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v1 and v2) (:chart_with_upwards_trend: Range-Diff between v1 and v2)

Checkout this PR version locally ```sh 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.
canihasreview[bot] commented 1 month ago

v3

@baskargopinath submitted v3 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v2 and v3) (:chart_with_upwards_trend: Range-Diff between v2 and v3)

Checkout this PR version locally ```sh 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 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

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