Conversation
The new filter is quite slow though, so we might want to add that as a BE python step
✅ Deploy Preview for radiant-cucurucho-d09bae ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
I'm seeing the out of memory issue again, so I think we need to add |
hunter-yeago
left a comment
hunter-yeago
left a comment
There was a problem hiding this comment.
Only thing that has to change is the x-scroll on mobile for the input of the search input (the one with width: 20rem;) - otherwise I've proposed a couple of changes to the code. Could go either way really though!
| // Non reporting isn't new | ||
| if (reportedYears.length === 0) { | ||
| // New buildings have exactly 1 year with reported data | ||
| if (yearsWithReportedData.length !== 1) { |
There was a problem hiding this comment.
Is it possible for this to be triggered if the building has only one year of reported data, but that is not the most recent year? For example, reported data only for 2019 but for no other year?
| @@ -41,13 +44,19 @@ export default class Search extends Vue { | |||
| BuildingBenchmarkStats; | |||
There was a problem hiding this comment.
On line 43
export default class Search extends Vue {
I am getting the following error:
A module cannot have multiple default exports.ts-plugin(2528)
... though I don't see any other default exports? Strange.
src/pages/Search.vue
Outdated
| !this.propertyTypeFilter && | ||
| !this.gradeFilter && | ||
| !this.gradeQuintileFilter | ||
| !this.gradeQuintileFilter && |
There was a problem hiding this comment.
If this used anywhere in the filter? I see gradeFilter being used, but this seems to be initialized without being used unless I'm missing something.
src/pages/Search.vue
Outdated
| /** | ||
| * Get all active filter configurations | ||
| */ | ||
| private getActiveFilters() { |
There was a problem hiding this comment.
This is my first time making a code change to a PR so forgive me if this breaches any QA etiquette.
Just wanted to propose an alternative solution for getting the active filters besides the if statements / ternarys. Feel free to roll this back if you prefer to keep the old way!
Either way, from my testing on the frontend, the filters are working great!
| </div> | ||
| </div> | ||
|
|
||
| <div class="checkbox-row"> |
There was a problem hiding this comment.
What do you think of moving these inputs to the same line as the Property Type / Grade Filter? It's a small thing but I feel like it would look a little nicer if all of the filters were on one line and maybe had a consistent style and wrapped only when the viewport gods demand. Certainly not a hill I'm willing to be smited on though ⚡
| gap: 0.5rem; | ||
| padding: 0.5rem 1rem; | ||
| input { | ||
| width: 20rem; |
There was a problem hiding this comment.
Causes x-scroll on mobile
may i suggest:
width: min(100%,20rem);
There was a problem hiding this comment.
Another thought - not pertinent to this PR but I wonder if there's a way we can make it a little clearer that there is more to see on this table on mobile. For example, we can have it that the X-scrollbar is always showing.
Or maybe at a certain viewport instead of it still being a scrollable table, instead you can click on the item and it'll show a box underneath with the rest of the information?
A bit of a bigger refactor so not saying to do that now, but yeah. It's just a big table to have to scroll through, especially if you want to view multiple buildings. Have to keep scrolling back and forth over and over again to see multiple buildings. Might be a faster way. But of course... we should verify that this is something that any users we have now are actually looking to have added before putting in the work, if we think we should...
Makes me think - for the people that you know who use the site - do they primarily do so through mobile or desktop?
|
@hunter-yeago - this is an old draft PR with some new search stuff, so I actually need to merge with |
|
@vkoves Ah. My bad. You know, they don't have schools in Florida. So I'm still learning how to read. Taking a look at the other PR now. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Talking in person, the expand collapse is a bit overkill, I should move the search stuff back to one line |
2 participants
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Testing Instructions
Please describe the tests/QA that you did to verify your changes. Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration
Checklist:
Data Update (if applicable):