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.

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

fix(#8686): update target UI to match figma #8687

Merged
merged 25 commits into from
Nov 16, 2023

Conversation

Benmuiruri
Copy link
Contributor

@Benmuiruri Benmuiruri commented Nov 8, 2023

Description

Update Target Page CSS to meet match Figma design

#8686

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

Compose URLs

If Build CI hasn't passed, these may 404:

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@Benmuiruri
Copy link
Contributor Author

Hi @latin-panda as you check out this draft PR, one change that is proving challenging to reproduce exactly is aligning the icon to the text.

  1. The icons in the figma design have different measurements yet in the code we have fixed measurements
  2. The icons in the cards with a h2 that spans 2 lines are not perfectly aligned. I am not yet sure how to target such h2's because they are coming in from <h2 *ngIf="target.translation_key">{{ target.translation_key | translate }}</h2>. It's hard to style not knowing what the value of the translation key will be. Will be 1 line, 2 line, 3 lines ?

Let me know your thoughts on it.

What I have done so far

Tasks to complete this work:

  • Should keep margins in responsive design (small phones (360 x 670), regular phones, small desktops (10"), and regular desktops (15"))
  • Should have a distinctive background color from the page's background
  • Should have radius in corners.
  • Should align icons when text next to it is long or short - (Partially)

What's left

  • Should align the small texts (goals)
  • Verify the target page matches the Figma design when having different device sizes. Attach test evidence in the PR.
  • Update documentation's screens, link CHT Docs PR here.

@Benmuiruri
Copy link
Contributor Author

Benmuiruri commented Nov 8, 2023

Fixes done so far

  • The margin around the card and other elements is 10px (top and bottom), 12px (left and right). The cards should keep that margin as they extend horizontally (width)
Screenshot 2023-11-08 at 16 50 02
  • The icon is aligned top in relation to the text. For both titles that span 1 line and titles that span 2 lines
Screenshot 2023-11-08 at 16 52 10 Screenshot 2023-11-08 at 16 52 29
  • Added a new grey variable and updated the target card background color from #F2F2F2 to #F8F8F8
  • Updated border-radius to 8px from 10px
Screenshot 2023-11-08 at 16 54 01
  • The small text is aligned to the progress-bar. It is center aligned unless the goal is completed and in that case it is right aligned.
  • Added margin below the calculation (100% 1 of 1)
  • Updated the font weight from bold to normal
Screenshot 2023-11-08 at 17 01 10 Screenshot 2023-11-08 at 17 08 00 Screenshot 2023-11-08 at 17 12 17

The one card I was not able to reproduce is this one below. I am not sure which kind of object I should add to target.js to achieve that. Perhaps @latin-panda you could help with this one.
Screenshot 2023-11-08 at 17 13 04

@n-orlowski This is ready for your review incase something is not pixel-perfect based on the Figma design. Any tips on how to see the actual pixel count between elements ? For instance, how can I tell the padding value around the inner rectangle from the outer one ?

Screenshot 2023-11-08 at 17 39 24

@Benmuiruri Benmuiruri marked this pull request as ready for review November 8, 2023 14:21
@n-orlowski
Copy link

Thanks Ben, these look great! A few comments:
Screen Shot 2023-11-08 at 10 00 06 AM
Goal 0 should be 20px from top and right corner
The padding under This month should be 20px (same as the space above Deaths)

Screen Shot 2023-11-08 at 10 02 43 AM

The small text is aligned to the progress-bar. It is center aligned unless the goal is completed and in that case it is right aligned.

If the goal is on the lower end, have the text be left aligned. Also, the progress bar corners should all be rounded (on the right side of the bar it's squared). Lastly, the space above and below in facility and all time should be 20px

Screen Shot 2023-11-08 at 10 04 12 AM

I'm not sure if this is a visual illusion but it seems like the space above and below the large numbers are different; there should be 20px on top and bottom

I think that's it! Thanks again for these updates 🙏

@Benmuiruri
Copy link
Contributor Author

Hi @latin-panda, I have addressed Nicole's requested changes, this PR is ready for a thorough review. If there are any changes required, do you have any tips / tools I can use to see the actual pixel count between elements ? For instance, how can I tell the padding value around the inner rectangle from the outer one ?

Screenshot 2023-11-08 at 17 39 24

Or how can I tell the padding value between the goal and the top of the card should be 20px.
Screenshot 2023-11-09 at 09 00 37

Copy link
Contributor

@latin-panda latin-panda left a comment

Choose a reason for hiding this comment

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

Thanks Ben! A couple of feedback. I'm going to run the branch in my local in the meantime.

webapp/src/css/targets.less Outdated Show resolved Hide resolved
webapp/src/css/targets.less Outdated Show resolved Hide resolved
@latin-panda
Copy link
Contributor

@Benmuiruri please fix the PR title to match the format fix(<ticket>): <title>

@Benmuiruri Benmuiruri changed the title 8686 update target UI to match figma fix(#8686): update target UI to match figma Nov 9, 2023
Copy link
Contributor

@latin-panda latin-panda left a comment

Choose a reason for hiding this comment

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

@ben I left a comment. Also, please mark the completed items from this checklist.
The documentation is a good way to detect bugs

Copy link
Contributor

@latin-panda latin-panda left a comment

Choose a reason for hiding this comment

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

Looks good, please update the checklist in this ticket with the completed items.
And once the CI is green, you can do squash and merge. Approving now to unblock

@Benmuiruri Benmuiruri force-pushed the 8686-update-target-ui-to-match-figma branch from ab7c525 to f49580c Compare November 10, 2023 13:17
@Benmuiruri Benmuiruri mentioned this pull request Nov 10, 2023
8 tasks
@Benmuiruri
Copy link
Contributor Author

Hi @garethbowen and @1yuv this PR has been completed, tested, and approved. It's ready to be merged once the CI build passes.

@latin-panda
Copy link
Contributor

@Benmuiruri The fix for e2e has been merged, please update this PR with the latest from the main branch. Thanks!

@Benmuiruri Benmuiruri force-pushed the 8686-update-target-ui-to-match-figma branch from f49580c to fe68b4d Compare November 15, 2023 14:24
@Benmuiruri Benmuiruri merged commit ee24680 into master Nov 16, 2023
36 checks passed
@Benmuiruri Benmuiruri deleted the 8686-update-target-ui-to-match-figma branch November 16, 2023 03:46
Benmuiruri added a commit that referenced this pull request Nov 16, 2023
* Chore: add border-radius and gray-bg variables

* Fix: update target border-radius and gray-bg

* Fix: update target card margins on mobile

* Fix: add horizontal border in target card

* Fix: align icon with text

* Fix: match Figma design

* Fix: small adjustment

* Fix: adjust padding

* Fix: adjust progress bar border

* Fix: adjust goal text alignment

* Fix: create variables

* Fix: rename variable

* Fix: update progress bar in target aggregate

* Fix: minor adjustment

* Fix: adjust progress bar

* Fix: adjustments

* Fix: adjustments

* Fix: adjustments

* Fix: adjustments

* Fix: adjustments

* Fix: tiny adjustment

* Fix: tiny adjustment

* Fix: extract class

* Fix: target aggregate adjustment

* Fix: progress bar adjustment
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.

None yet

3 participants