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

DEV - Update styling of the Service Request details modal #1870

Open
14 tasks done
Tracked by #1862
traycn opened this issue Nov 22, 2024 · 20 comments · May be fixed by #1906
Open
14 tasks done
Tracked by #1862

DEV - Update styling of the Service Request details modal #1870

traycn opened this issue Nov 22, 2024 · 20 comments · May be fixed by #1906
Assignees
Labels
Milestone

Comments

@traycn
Copy link
Member

traycn commented Nov 22, 2024

Overview

We need to update the Service Request detail modal to more clearly indicate the Neighborhood Council pertaining to the Service Request

Action Items

  • Update the Styling of the Service Request details to reflect the mockups of the modal
    • Updated SR title: type icon (e.g. colored circle) is now left of the SR detail title, content is aligned center
    • Updated rounded corners: 30px
    • Updated color for title section bar: #000000
    • Updated title styles: Roboto, bold, 40px
    • Updated section header styles: Roboto, regular, 24px
    • Updated section details styles: Roboto, thin, 16px, #FFFFFF (looks grey but its not), 18px padding-left
    • Updated "Service Request" field name: Roboto, SemiBold, 16px, underline
    • Updated "Service Request" field value: "^, text is padded with a pound sign and non-breaking space: # 1-458940111
    • Updated "Reported On" field name and value: Roboto, Light, 14px, #FFB104
    • Updated "Closed" field name and value: "^ NOTE: this field only exists on SRs that report a Closed date -- otherwise do not render this field
    • Updated "Source" field name and value: Roboto, Thin, 10px, #FFFFFF (also looks grey but its not)
    • Updated "Agency" field name and value: "^
    • SR Detail component has the following layout sections
      • Location, Neighborhood council: this is similar to previous version, except now it explicitly has a field name and value, value is indented
      • Service Request: this layout is identical to the previous version (e.g. table, field name is aligned left, value is aligned right)
      • Reported, Closed: these have an identical layout to Location, Neighborhood. Re-use this layout if possible
      • Source, Agency: these have an identical layout to the table used in the previous version, except Source is the left column and Agency is the right column. Attempt to re-use code if possible. This section is 64px from the bottom. (see resources)

Resources/Instructions

Screenshot before proposed changes

Screen Shot 2024-11-21 at 7 13 26 PM

Screenshot after proposed changes

This is achieved in Figma by doing the following:

  1. double clicking on an element
  2. holding option key (Mac) or alt key (Win)
  3. mouse over another element, which will bring up the red guide-lines that shows the distance relative to the element selected in Step 1.

image

[NEW] added per request from comment

Pixel length of Source/Agency to bottom of component

Image

@traycn traycn self-assigned this Nov 22, 2024
@github-project-automation github-project-automation bot moved this to New Issue Approval in P: 311: Project Board Nov 22, 2024
@traycn traycn added this to the 04 - Map Page milestone Nov 22, 2024
@traycn traycn changed the title Update Service Request details modal Update styling of the Service Request details modal Nov 22, 2024
@ryanfchase
Copy link
Member

ryanfchase commented Jan 3, 2025

@traycn I am modifying this ticket slightly:

  • Overview
    • We need to do update the Service Request detail modal to refresh the user experience when viewing the Service Request details. more clearly indicate the Neighborhood Council pertaining to the Service Request
    • Reference to the Design issue: #1807 (Comment) ⬇️ move this down, to Resources
  • Resources / Instructions

edit: mixup fixed

@ryanfchase
Copy link
Member

ryanfchase commented Jan 3, 2025

@Joy-Truex @allisonjeon I've put together the list of Action Items the dev will need to make to implement the design. I'm needing help confirming the exact specs for a couple of points:

Q1

  • Updated section details styles: Roboto, thin, 16px, #FFFFFF (looks grey but its not), 19px padding-left

I found 19px by looking inspecting the X position for Location, and comparing it to the X position for 7038 N Vesper Ave. 91405. They are 19 pixels in X position. Is there a better way to find the correct padding that was used? Or is this precise enough?

Q2

  • Updated "Closed" field name and value: "^ NOTE: this field only exists on SRs that report a Closed date -- otherwise do not render a date if it does not exist

I note that "Closed` will not appear on every SR, only SRs that have been closed. Is my suggestion good? Or is there a better way to handle SRs that don't have a Closed Date?

Q3

  • Source, Agency: these have an identical layout to the table used in the previous version, except Source is the left column and Agency is the right column. Attempt to re-use code if possible. This section is ~82px from the bottom.

Similar question as Q1. I am deducing 82px by inspecting Y position and doing math. Let me know if there is a more precise way to do this.

Thanks!

@ryanfchase ryanfchase added the Question Further information is requested label Jan 3, 2025
@ryanfchase ryanfchase moved this from New Issue Approval to Questions in P: 311: Project Board Jan 3, 2025
@ryanfchase ryanfchase added the ready for design lead ready for design lead to review the issue label Jan 3, 2025
@ryanfchase
Copy link
Member

@traycn if the Action Items look OK to you, and once we get a follow up from UIUX leads, I think we are good to move this ticket into PrioBacklog

@ryanfchase ryanfchase added ready for dev lead ready for developer lead to review the issue and removed draft labels Jan 3, 2025
@Joy-Truex
Copy link
Member

Joy-Truex commented Jan 9, 2025

Hey @ryanfchase!

Q1

  • Updated section details styles: Roboto, thin, 16px, #FFFFFF (looks grey but its not), 19px padding-left

I found 19px by looking inspecting the X position for Location, and comparing it to the X position for 7038 N Vesper Ave. 91405. They are 19 pixels in X position. Is there a better way to find the correct padding that was used? Or is this precise enough?

An easy way to find this is to highlight object and hover over the second object while holding down the option key (mac).
This will yield the following result:

Distance Between Layers Screenshot

Screenshot 2025-01-09 at 10 33 43 AM

Q2

  • Updated "Closed" field name and value: "^ NOTE: this field only exists on SRs that report a Closed date -- otherwise do not render a date if it does not exist

I note that "Closed` will not appear on every SR, only SRs that have been closed. Is my suggestion good? Or is there a better way to handle SRs that don't have a Closed Date?

Yes thats perfectly fine.

Q3

  • Source, Agency: these have an identical layout to the table used in the previous version, except Source is the left column and Agency is the right column. Attempt to re-use code if possible. This section is ~82px from the bottom.

Similar question as Q1. I am deducing 82px by inspecting Y position and doing math. Let me know if there is a more precise way to do this.

Same here!!

Distance Between Layers Screenshot

Screenshot 2025-01-09 at 10 46 04 AM
Screenshot 2025-01-09 at 10 46 14 AM

Thanks!

I'm gonna go ahead and remove the "ready for design lead" label, please tag me again if any other questions come up :)

@Joy-Truex Joy-Truex removed the ready for design lead ready for design lead to review the issue label Jan 9, 2025
@DorianDeptuch
Copy link
Member

Availability: Weekday Evenings
ETA: 1/21 End of Day

@ryanfchase ryanfchase moved this from Prioritized Backlog to In progress in P: 311: Project Board Jan 15, 2025
@DorianDeptuch
Copy link
Member

DorianDeptuch commented Jan 18, 2025

Blockers:

  1. [Resolved] The border-radius style exists on Map.jsx, which is the the parent component of RequestDetail.jsx, among other modal components. As a result, adding a border-radius of 30px will also cause the loading modal that precedes RequestDetail.jsx, and possibly other modal components that share this property, to have a border-radius of 30px.
Photo of loading modal with 30px border-radius

Image

  1. [Resolved] The Action Items mention that there are ~82 pixels from the Source/Agency to the bottom of the modal, however, I cannot seem to find the measurements for the remaining margin/padding/distances between the remaining elements. Should I experiment with different measurements to attempt to replicate the final design, or wait for updated measurements from the design team?
Photo of RequestDetail modal with Action Items completed

Image

  1. [Resolved] This is a small issue, but I wanted to be sure it wasn't a mistake: In the screenshot before proposed changes and in the codebase, the field name is "Reported On." In the screenshot after proposed changes, the field name is "Reported." Which one are we going to stick with?

@traycn
Copy link
Member Author

traycn commented Jan 18, 2025

Hey @Joy-Truex !

Could you provide some insight into Dorian's questions from the previous comment. Summarized below:

  1. Does the team have a mockup for the Loading modal (screenshot in 1. above)?

  2. Can you share a screenshot of the padding/margin values from the modal mockup in Figma? Feel free to attach it to a comment in this thread.

  3. Does design prefer "Reported" or "Reported On" for the date in orange text?

@ryanfchase

This comment has been minimized.

@ryanfchase
Copy link
Member

ryanfchase commented Jan 19, 2025

@DorianDeptuch I've addressed point 2 by providing a screenshot and instructions on finding pixel distances. You can find this in Resources of main ticket. I've also updated 2 of the Action Items:

  • ... This section is 82px 64px from the bottom.
  • Updated section details styles: Roboto, thin, 16px, #FFFFFF (looks grey but its not), 19px padding-left 18px padding-left

Feel free to mark this as resolved if this clarifies your question.

@ryanfchase ryanfchase added Question Further information is requested ready for design lead ready for design lead to review the issue labels Jan 19, 2025
@ryanfchase ryanfchase moved this from In progress to Questions in P: 311: Project Board Jan 19, 2025
@Joy-Truex
Copy link
Member

Joy-Truex commented Jan 20, 2025

@traycn some replies for ya!

  1. Does the team have a mockup for the Loading modal (screenshot in 1. above)?

can you please specify what the modal is for? i'm not sure that we do.

  1. Can you share a screenshot of the padding/margin values from the modal mockup in Figma? Feel free to attach it to a comment in this thread.

addressed by Ryan, ty!!

  1. Does design prefer "Reported" or "Reported On" for the date in orange text?
    Please use "Reported"

Design prefers this as it's more concise and therefore more scannable in this instance

@traycn
Copy link
Member Author

traycn commented Jan 21, 2025

Hey @Joy-Truex !

  1. The SR modal displays a Loading spinner as we gather the SR details.

The current look of the Loading spinner in the modal is below:

  1. The border-radius style exists on Map.jsx, which is the the parent component of RequestDetail.jsx, among other modal components. As a result, adding a border-radius of 30px will also cause the loading modal that precedes RequestDetail.jsx, and possibly other modal components that share this property, to have a border-radius of 30px.

Photo of loading modal with 30px border-radius
Image

Can design provide a mockup of the modal with a Loading spinner for dev to implement?

@traycn
Copy link
Member Author

traycn commented Jan 21, 2025

Following up on the SR date title's text @DorianDeptuch !

We'll be using the "Reported" text for the SR date value.

@ryanfchase
Copy link
Member

Per conversation with dev and design lead, the design of the SR detail's loading modal should remain unchanged. Only use a border radius of 30px for the SR details modal, as demonstrated in the mockup.

Screenshot of SR detail's loading modal

  • Observe that the SR detail's loading modal has a border radius of 5px.

Image

Turning this back over to @DorianDeptuch and moving to In Progress. If there are further questions, leave them in a comment. Don't forget to include the ready for dev lead and Question label, and move it to the Questions column for visibility. Thanks!

@ryanfchase ryanfchase removed Question Further information is requested ready for design lead ready for design lead to review the issue labels Jan 22, 2025
@ryanfchase ryanfchase moved this from Questions to In progress in P: 311: Project Board Jan 22, 2025
@DorianDeptuch
Copy link
Member

DorianDeptuch commented Jan 22, 2025

Updated ETA: 1/24 End of Day

@DorianDeptuch
Copy link
Member

DorianDeptuch commented Jan 29, 2025

Blockers:

[Resolved] I can't seem to apply dynamic styling to the correct div when attempting to add border-radius to the modal component.

<div> // This is the mapContainer
    <OtherComponent />
    <OtherComponent />
    <OtherComponent />
    <OtherComponent />
    <OtherComponent />
    <div> // This outer div is created somewhere in the code and is the desired target to add dynamic border-radius styling to
         <div 
              style={{ borderRadius: isRequestDetailLoading ? '5px' : '30px', border: '5px solid red' }}
              ref={(el) => (this.requestDetail = el)}
         >
              <RequestDetail 
                  requestId={selectedRequestId} 
                  loadingCallback={this.setRequestDetailLoading}
              />
        </div>
    </div>
</div>
A screenshot of the rendered modal

Image

'& .mapboxgl-popup-content': {
      width: 'auto',
      backgroundColor: theme.palette.primary.main,
      borderRadius: 5, // This controls the border-radius for the aforementioned outer div
      padding: 10,

One idea is to add dynamic styling within the .mapboxgl-popup-content class, though I'm not sure if that's possible from what I've read about themes. My hunch is the addPopup method in Map.jsx might have some control over this div but haven't been able to get the classes to work as is described in the mapboxgl.Popup() documentation.

@ryanfchase
Copy link
Member

ryanfchase commented Jan 29, 2025

One idea is to add dynamic styling within the .mapboxgl-popup-content class, though I'm not sure if that's possible from what I've read about themes. My hunch is the addPopup method in Map.jsx might have some control over this div but haven't been able to get the classes to work as is described in the mapboxgl.Popup() documentation.

@DorianDeptuch You were on the right trail! I was able to utilize popup's addClassName method to add a class to the popup. However, I found that the class is inserted into the modal's container (see references). I then targeted .mapboxgl-popup-content using the mui styles syntax for nested classes (as seen throughout our codebase).

LMK if you need me to provide more instructions on how to incorporate my fix into your branch. I didn't use your branch because I wanted to ensure there were no bugs introduced by any of your code that I was unfamiliar with. cc @traycn !

Resources

Demo Branch

  • https://github.com/hackforla/311-data/tree/bugfixing-styling-help-1870
  • note: while attempting to try out solutions, I was experimenting with a separate component for the Loading spinner. This ended up not being a hard requirement, as calling this.popup.setDOMContent() simply updates the content, not the actual modal. In the end, adding classname was sufficient and there aren't any restrictions on where/when it can be done.
"testing" class gets added to the modal's container

Code (see note about "separate" modal)
<div ref={(el) => (this.requestDetailSpinner = el)}> 
  <RequestDetailLoader requestId={selectedRequestId} loadingCallback={() => {
    if (this.popup) {
      this.popup.setDOMContent(this.requestDetail);
      this.popup.addClassName(classes.testing);
    }
}}/>
Highlighting the modal container in dev tools, see divs with classes Map-testing-27 mapbox-gl-popup.

Image

`.mapboxgl-popup-content` targets the actual modal

Code for nested class
const styles = (theme) => ({
  root: {
    // ...
    testing: {
      // borderRadius: 30, // not useful!
      '& .mapboxgl-popup-content': {
        width: 'auto',              
        backgroundColor: theme.palette.primary.main,
        borderRadius: 30,
        padding: 10,
      },
    },
    // ...
  }
});
Border Radius of 30px being applied

Image

Video Demo

Sorry, timescodes ~9sec thru ~12sec is me struggling to trigger a breakpoint, sigh
border-radius.mp4

@DorianDeptuch DorianDeptuch linked a pull request Jan 30, 2025 that will close this issue
4 tasks
@traycn traycn changed the title Update styling of the Service Request details modal DEV - Update styling of the Service Request details modal Jan 30, 2025
@DorianDeptuch
Copy link
Member

Thanks for the help @traycn & @ryanfchase

PR created #1906

@DorianDeptuch DorianDeptuch moved this from In progress to Done (without merge) in P: 311: Project Board Jan 30, 2025
@ryanfchase
Copy link
Member

ryanfchase commented Jan 31, 2025

Provided feedback and addressed how your changes interact with a pretty moderate bug:

@DorianDeptuch please have a look at the feedback, decide if you're ok to address it -- if so leave an ETA for completion.

I also want to bring this up at product planning because I think we can also address the problem through design. I want to float the idea of "only bringing up SR details on-click", which would drastically reduce the number of accidental db requests.

@ryanfchase ryanfchase moved this from Done (without merge) to In progress in P: 311: Project Board Jan 31, 2025
@DorianDeptuch
Copy link
Member

Availability: 2/1 - 2/2: Evenings, 2/11 and Onward: Evenings.
ETA: 2/18 End of Day

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In progress
Development

Successfully merging a pull request may close this issue.

4 participants