-
Notifications
You must be signed in to change notification settings - Fork 0
Text tweaks from public feedback #149
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
Conversation
| HOMEVAL_ASSESSMENT_CARD_TABLE = "pinval.vw_assessment_card" | ||
| HOMEVAL_COMP_TABLE = "pinval.vw_comp" | ||
| HOMEVAL_DATA_DICT_TABLE = "pinval.vars_dict" | ||
| HOMEVAL_DATA_DICT_TABLE = "z_ci_update_vars_dict_bldg_sqft_desc_pinval.vars_dict" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder: Revert before merging
jeancochrane
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Unfortunately, I am yet again second guessing the exact phrasing of these headers... tagging in @ccao-jardine for feedback on my feedback as well. I don't think it's a huge deal, and I don't necessarily think we need to redeploy the reports even if we do change these two headers, because I could easily just tweak the HTML in a live report to see what it looks like.
|
|
||
| <!-- Comp map --> | ||
| <h2 class="mb-3">Top 5 Most Significant Sales</h2> | ||
| <h2 class="mb-3">The Most Significant Sales</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Nitpick, optional] Sorry for changing my mind yet again, but looking at it again today, this reads a bit weird to me -- kind of vague and mysterious, sort of like a book title more than a section header. Adding slightly more context makes it sound better to me, but maybe I'm overthinking it. What do you think?
| <h2 class="mb-3">The Most Significant Sales</h2> | |
| <h2 class="mb-3">The Most Significant Sales for Your Home</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I concur -- while our work can be mysterious and important, HomeVal should be clear and accessible. Thanks for flagging this! I do think that it is an improvement over "Top 5", but I'll join your overthinking.
Hmm. Now that I've reviewed this in context, I'm reminded that this header is also the first part of the report that even mentions significant sales, outside of the expandable "Click or tap to read more about this report" accordion (which we shouldn't assume people will read through).
"The Most Significant Sales for Your Home" feels a bit awkward, like we should either mean "Sales of my home" (not the correct description of what is to follow) or sales that are important to a home (homes probably don't have feelings about sales).
Thoughts on "The Most Significant Sales Used to Estimate Your Home's Value"? It's appreciably longer, which I don't like, but it doesn't look as bad as I would have thought on mobile (below width: 360 pixels). It still feels a little clunky to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeancochrane, @wagnerlmichael: Let's revert. Let's keep "Top 5 Most Significant Sales" until we have substantial evidence from multiple users that it's confusing enough to change. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good by me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me! in that case, should we close this PR, and then merge ccao-data/data-architecture#926? @jeancochrane
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that works for me @wagnerlmichael 👍🏻
| id="sale-characteristics-{{ .card.card_num }}" | ||
| > | ||
| Characteristics for Top 5 Most Significant Sales | ||
| Characteristics for the Most Significant Sales |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Nitpick, optional] Similar to above, I'm feeling like "for" is not quite the right preposition here. "Of" sounds more natural to me, and matches the way we phrase the "summary" header below. Does that sound better to you too?
| Characteristics for the Most Significant Sales | |
| Characteristics of the Most Significant Sales |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seconded, nice catch!
Edit the language from 'top 5 significant sale' to 'significant sales'. The top 5 language could be confusing as it could be interpreted as referring to market value.
Dev PIN: 01011000040000
Closes part of #147