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

Added variable height for Admin neighborhood completion histogram and… #3575

Conversation

nschung28
Copy link
Collaborator

@nschung28 nschung28 commented Jun 27, 2024

Resolves #3565

Made neighborhood completion histogram height variable depending on the dataset and also increased the font size of the neighborhoods on the y-axis.

Testing instructions
  1. Load any city to test the histogram on the Admin page.
Things to check before submitting the PR
  • I've written a descriptive PR title.
  • I've added/updated comments for large or confusing blocks of code.
  • I've included before/after screenshots above.
  • I've asked for and included translations for any user facing text that was added or modified.
  • I've updated any logging. Clicks, keyboard presses, and other user interactions should be logged. If you're not sure how (or if you need to update the logging), ask Mikey. Then make sure the documentation on this wiki page is up to date for the logs you added/updated.
  • I've tested on mobile (only needed for validation page).

… increased font size of names of neighborhoods
…Webpage into 3565-admin-neighborhood-completion-histogram-variable-height
@misaugstad
Copy link
Member

@nschung28 there should definitely be screenshots for this!

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

I think that the y-direction is looking good! But I'm now seeing that we have a scroll in the x-direction in the cities that I tested, and that wasn't an issue before. Is it because of the increased font size or something? We should definitely get rid of the x-direction scrolling! Possibly it could help to orient the y-axis labels at a 45 degree angle?
Screenshot from 2024-06-28 11-44-20
Screenshot from 2024-06-28 11-47-19

$.getJSON('/adminapi/neighborhoodCompletionRate', function (data) {
// Determine height of the chart based on size of data
Copy link
Member

Choose a reason for hiding this comment

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

A reminder to end comments with a period, as per style guide 🙃

Copy link
Member

Choose a reason for hiding this comment

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

And instead of "size of data", it might be more clear to say "the number of neighborhoods"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry about that! I'll go back and fix this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll also take a look into the x-axis scrolling issue and get those changes pushed asap!

$.getJSON('/adminapi/neighborhoodCompletionRate', function (data) {
// Determine height of the chart based on size of data
var chartHeight = data.length * 30;
Copy link
Member

Choose a reason for hiding this comment

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

Might as well just do var chartHeight = 150 + 30 * data.length;

…Webpage into 3565-admin-neighborhood-completion-histogram-variable-height
… to remove overflow on the x-axis, and edited comments to be more descriptive
… to remove overflow on the x-axis, and edited comments to be more descriptive
… to remove overflow on the x-axis, and edited comments to be more descriptive
@nschung28
Copy link
Collaborator Author

Before (with 3 neighborhoods):
Screenshot 2024-07-02 at 4 35 20 PM

After (with 3 neighborhoods):
Screenshot 2024-07-02 at 4 41 50 PM

Before (with 1049 neighborhoods):
Screenshot 2024-07-02 at 4 38 29 PM

After (with 1049 neighborhoods):
Screenshot 2024-07-02 at 4 43 06 PM

Screenshot 2024-07-02 at 4 43 40 PM Screenshot 2024-07-02 at 4 43 53 PM

Note that in the after with 1049 neighborhoods the first screenshot is the top of the chart, the second is the middle, and the third is the bottom.

Copy link
Member

@misaugstad misaugstad 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, just one small change!

public/javascripts/Admin/src/Admin.js Outdated Show resolved Hide resolved
Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

Thanks for getting back to this, looks good!

@misaugstad misaugstad merged commit 213c760 into develop Aug 21, 2024
@misaugstad misaugstad deleted the 3565-admin-neighborhood-completion-histogram-variable-height branch August 21, 2024 17:27
@misaugstad misaugstad mentioned this pull request Aug 22, 2024
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.

Admin neighborhood completion histogram should have variable height
2 participants