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.

Sign up for GitHub

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

Filter bugfixes #848

Merged
merged 5 commits into from
May 7, 2024
Merged

Filter bugfixes #848

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b0a3ccc
fix incomplete techs bug
sarahfossheim May 1, 2024
1b42811
sort filterts alphabetically
sarahfossheim May 1, 2024
34c360d
keep ALL as a technology option
sarahfossheim May 1, 2024
a804399
show all technologies when choosing all categories
sarahfossheim May 1, 2024
a3ab289
revert sort order and add fallback for selected techs
sarahfossheim May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions src/js/components/filters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,13 @@ class Filters {
techSelector.before(errorMsg);
}

/* Get a list of technologies */
const techs = this.technologies;
const sortedTechs = techs.sort((a, b) => a.technology - b.technology);
Copy link
Member

Choose a reason for hiding this comment

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

Per #845 (comment) let's revert back to sorting by # sites

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 just revert back for now, and suggest we add more user-friendly dropdowns as a separate action item (so deciding on a sort order and design for it doesn't block the other bugfixes in this PR). If we keep it by origins, then we should implement a component more similar to the old report, where we can display the numbers as well.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM!

sortedTechs.unshift({ technology: 'ALL' });

/* Add one option per technology */
this.technologies.forEach((technology) => {
sortedTechs.forEach((technology) => {
const optionTmpl = document.getElementById('filter-option').content.cloneNode(true);
const option = optionTmpl.querySelector('option');
const formattedTech = technology.technology;
Expand All @@ -107,7 +112,8 @@ class Filters {
updateGeo() {
const select = document.querySelector('select#geo');
select.innerHTML = '';
this.geos.forEach((geo) => {
const sortedGeos = this.geos.sort((a, b) => a.geo !== b.geo ? a.geo < b.geo ? -1 : 1 : 0);
Copy link
Member

Choose a reason for hiding this comment

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

Per #845 (comment) let's revert back to sorting by # sites

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See: #848 (comment)

For geos I'm wondering if alphabetical might make it easier to find specific countries, as that's how most country selectors on the web are sorted alphabetically. If I want to find Norway, I know I still have to scroll further when I see "Finland" but am close when I see "Netherlands" or "North Korea". Maybe a good one to look for some more feedback/user tests on, or try out two different designs?

Copy link
Member

Choose a reason for hiding this comment

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

I do also like how the Data Studio select list includes a search filter. More complex to reimplement but could give us the best of both.

image

sortedGeos.forEach((geo) => {
const optionTmpl = document.getElementById('filter-option').content.cloneNode(true);
const option = optionTmpl.querySelector('option');
const formattedTech = geo.geo;
Expand Down Expand Up @@ -150,7 +156,8 @@ class Filters {
all.innerHTML = 'ALL';
select.append(all);

this.categories.forEach((category) => {
const sortedCategories = this.categories.sort((a, b) => a.category !== b.category ? a.category < b.category ? -1 : 1 : 0);
sortedCategories.forEach((category) => {
const option = document.createElement('option');
option.value = category.category;
option.innerHTML = category.category;
Expand All @@ -162,18 +169,21 @@ class Filters {

/* Set the selected category */
updateCategory(event) {
const selectedTechs = this.categories.find(category => category.category === event.target.value)?.technologies;
const selectedTechInfo = [];
selectedTechs.forEach(selectedTech => selectedTechInfo.push(this.technologies.find(tech => tech.technology === selectedTech)));
// Get the techs associated with the selected category
const selectedCategory = this.categories.find(category => category.category === event.target.value);
const selectedTechs = selectedCategory?.technologies;
Copy link
Member

Choose a reason for hiding this comment

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

I think we need a fallback in case selectedCategory is null, to avoid breaking the forEach below

Suggested change
const selectedTechs = selectedCategory?.technologies;
const selectedTechs = selectedCategory?.technologies || [];

console.log(selectedCategory, selectedTechs);

// Get the component with the selected tech
const techSelector = document.getElementById(event.target.dataset.tech);
techSelector.innerHTML = '';

selectedTechInfo.forEach((technology) => {
// Update the options
selectedTechs.forEach((technology) => {
const option = document.createElement('option');
const formattedTech = technology.technology;
option.textContent = technology.technology;
option.value = formattedTech;
if(formattedTech === techSelector.getAttribute('data-selected')) {
option.textContent = technology;
option.value = technology;
if(technology === techSelector.getAttribute('data-selected')) {
option.selected = true;
}
techSelector.append(option);
Expand Down
1 change: 0 additions & 1 deletion src/js/techreport/summaryCards.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ class SummaryCard {
const scoreCategoryName = scoreCategory?.name;
circle.setAttribute('style', `--offset: ${100 - latestValue};`);
const chart = card.querySelector('svg.progress-chart');
console.log('chart', chart);
chart.classList.add(scoreCategoryName);
});

Expand Down
Loading