codeforboston / jobhopper

Jobhopper helps planners and career coaches analyze patterns of career-changers to inform policy and planning decisions. Contact jobhopper@codeforboston.org if you have issues you would like to report.
MIT License
11 stars 26 forks source link

Text and Layout Changes #338

Closed jedpittman closed 3 years ago

jedpittman commented 3 years ago

Ensure that the latest changes from @sashamaryl are integrated and that you're getting the last code from develop before starting since some of these may not be needed.

Fix the items below:

  1. Change results sentence, remove wage source (we mean the state. The state should not be in the results themselves.)
  2. Space between “Transition Share:” and percentage must match space between “Salary:” and “Hourly”
  3. Footnote text (reviewed by AS): The Treemap above shows the occupations which Marketing Managers move to when they change occupation based on the (show number if possible, ie 10,000) observations in our dataset. The transition share is the percentage of these observed Marketing Managers who have moved into each of the occupations listed.
  4. Align left to Treemap edge, match text size of “What do xxx transitions to?”

For Review, please have Alex or Jed review the changes. Reach out them in slack and leave a note in the comments. Once someone reviews, make a comment in the ticket that this looks good and is approved. Another team member will then merge.

A good review will: Ensure all bullets are done. Only files/changes to files are related to the changes described. Ensure the code runs on your machine locally and works as expected. Ensure that the tests run on your machine locally There are no unresolved conflicts/errors with tests for the merge request.

edward-malouf commented 3 years ago

LInk to google doc with punchlist https://docs.google.com/document/d/1TvifH_DTHGkNUrGF1G5ilhkBGAtC9o_AYIKbxiTYWNQ/edit

edward-malouf commented 3 years ago

PDF with a visual key to the above punchlist Table data layout.pdf

jkoren commented 3 years ago

For item 2 - Space between “Transition Share:” and percentage must match space between “Salary:” and “Hourly”

The space between "Transition Share" and the Percentage seems to match the PDF Visual Key. Does any more need to be done?

Screenshot below. image

edward-malouf commented 3 years ago

Jeff,

This issue regarding the Treemap, note the table. See the PDF of the google doc and visual key attached.

The table has one issue, the state must appear in the lower left corner and be taken out of the results sentence.

I am here to answer any question and to help decide what is required for our MVP.

Thanks!

Ed

Content • Design Collaborative LLC 433 Country Way Scituate, MA 02066 781-378-1484

Bonding over wallpaper http://contentdesign.me/historical-finishes/

On Mar 11, 2021, at 2:29 PM, Jeff Korenstein @.***> wrote:

For item 2 - Space between “Transition Share:” and percentage must match space between “Salary:” and “Hourly”

The space between "Transition Share" and the Percentage seems to match the PDF Visual Key. Does any more need to be done? https://user-images.githubusercontent.com/67333843/110842758-86d7b480-8275-11eb-990c-0027e39c1086.png — You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/codeforboston/jobhopper/issues/338#issuecomment-796986125, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOG6WTYSM6QTTTKOEYY757DTDEDX3ANCNFSM4Y4YZRYA.

edward-malouf commented 3 years ago

I just noticed the visual key and text do not match.

Let me fix this.

Ed

UX/UI http://edmalouf.me/

Content•Design http://contentdesign.me/

On Mar 11, 2021, at 2:29 PM, Jeff Korenstein @.***> wrote:

For item 2 - Space between “Transition Share:” and percentage must match space between “Salary:” and “Hourly”

The space between "Transition Share" and the Percentage seems to match the PDF Visual Key. Does any more need to be done? https://user-images.githubusercontent.com/67333843/110842758-86d7b480-8275-11eb-990c-0027e39c1086.png — You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/codeforboston/jobhopper/issues/338#issuecomment-796986125, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOG6WTYSM6QTTTKOEYY757DTDEDX3ANCNFSM4Y4YZRYA.

edward-malouf commented 3 years ago

Up to date visual reference to the google doc.

On Mar 11, 2021, at 2:29 PM, Jeff Korenstein @.***> wrote:

For item 2 - Space between “Transition Share:” and percentage must match space between “Salary:” and “Hourly”

The space between "Transition Share" and the Percentage seems to match the PDF Visual Key. Does any more need to be done? https://user-images.githubusercontent.com/67333843/110842758-86d7b480-8275-11eb-990c-0027e39c1086.png — You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/codeforboston/jobhopper/issues/338#issuecomment-796986125, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOG6WTYSM6QTTTKOEYY757DTDEDX3ANCNFSM4Y4YZRYA.

jkoren commented 3 years ago

Hi Ed, I completed items #1 and #3. In #3 I will look to see if there is a straightforward way to include the number of observations.

2 - Thanks for the note that this refers to the Treemap. I understand we want an additional space between the "Transition Share" and the percentage in the text line under the Treetable, so I added that.

4 - The “What do xxx transitions to?” table and the Treemap text size is different because for the table, the the text is a title in a Material-UI table and the in the Treemap the text is an HTML title. Let me see if I can fix that.

As an aside, I'm not sure if there is a connection between the letters on the visual reference and the item numbers on the google doc?

Reach out to me at 617-590-0528 if I got anything wrong!

jkoren commented 3 years ago

@jedpittman a test failed when I ran npm test because the test is still looking for the state. Let me know if I should update the test.

       99 |     await selectEvent.select(getByLabelText('state-select'), 'California');
      100 | 
    > 101 |     await waitFor(() =>
          |           ^
      102 |       expect(getByText(/in California/i)).toBeInTheDocument()
      103 |     );
      104 | 
edward-malouf commented 3 years ago

Thanks Jeff!

4 is just the type of refinement that is NOT required for MVP.

Ed

Content • Design Collaborative LLC 433 Country Way Scituate, MA 02066 781-378-1484

Mills Along the Blackstone http://contentdesign.me/20-years-along-blackstone-river/

On Mar 11, 2021, at 5:02 PM, Jeff Korenstein @.***> wrote:

Hi Ed, I completed items #1 https://github.com/codeforboston/jobhopper/pull/1 and #3 https://github.com/codeforboston/jobhopper/issues/3. In #3 https://github.com/codeforboston/jobhopper/issues/3 I will look to see if there is a straightforward way to include the number of observations.

2 https://github.com/codeforboston/jobhopper/pull/2 - Thanks for the note that this refers to the Treemap. I understand we want an additional space between the "Transition Share" and the percentage in the text line under the Treetable, so I added that.

4 https://github.com/codeforboston/jobhopper/issues/4 - The “What do xxx transitions to?” table and the Treemap text size is different because for the table, the the text is a title in a Material-UI table and the in the Treemap the text is an HTML title. Let me see if I can fix that.

As an aside, I'm not sure if there is a connection between the letters on the visual reference and the item numbers on the google doc?

Reach out to me at 617-590-0528 if I got anything wrong!

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/codeforboston/jobhopper/issues/338#issuecomment-797079279, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOG6WT23L27C3L6IVQXUXSLTDEVVBANCNFSM4Y4YZRYA.

jedpittman commented 3 years ago

@jedpittman a test failed when I ran npm test because the test is still looking for the state. Let me know if I should update the test.

       99 |     await selectEvent.select(getByLabelText('state-select'), 'California');
      100 | 
    > 101 |     await waitFor(() =>
          |           ^
      102 |       expect(getByText(/in California/i)).toBeInTheDocument()
      103 |     );
      104 | 

Yes, the tests should all be in a working state prior to check-in/merge request. If any tests start to fail as a result of the changes to the code; they should be updated. Also if new changes are added to the code, that is the correct time to add tests to make sure we have coverage for new code.

jkoren commented 3 years ago

Tests passed. Pushed and created reviews and pull request. We considered adding clarifying language to the titles as below, but decided to hold off because this is covered in the Tool Tip. image image