Skip to content

Commit 64eaa4f

Browse files
tobiutobiu
committed
Fix Helix & Gallery selection models keynav and record resolution (#9367)
- Update createItem to use getRecordId instead of hardcoding getKey, respecting useInternalId - Update onSelect to map the internalId back to the country string to keep the URL hash clean - Update afterSetCountry to resolve the string value to its internalId to accurately detect external vs internal selection changes, fixing the false-positive 1s animation fireworks
1 parent fce8270 commit 64eaa4f

4 files changed

Lines changed: 88 additions & 42 deletions

File tree

‎apps/covid/view/country/Gallery.mjs‎

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -92,22 +92,32 @@ class CountryGallery extends Gallery {
9292
*/
9393
store: CountryStore
9494
}
95-
96-
/**
97-
* Triggered after the country config got changed
98-
* @param {String|null} value
99-
* @param {String|null} oldValue
100-
* @protected
101-
*/
102-
afterSetCountry(value, oldValue) {
103-
if (oldValue !== undefined) {
104-
let selectionModel = this.selectionModel;
105-
106-
if (value && !selectionModel.isSelected(value)) {
107-
selectionModel.select(value, false);
95+
/**
96+
* Triggered after the country config got changed.
97+
* We need to distinguish between manual view-based selections (like keynav or clicks) and
98+
* programmatic selections that originate from other widgets (like the combobox or hash change).
99+
* @param {String|null} value
100+
* @param {String|null} oldValue
101+
* @protected
102+
*/
103+
afterSetCountry(value, oldValue) {
104+
if (oldValue !== undefined) {
105+
let me = this,
106+
selectionModel = me.selectionModel,
107+
recordId = value;
108+
109+
if (me.useInternalId && value) {
110+
let record = me.store.get(value);
111+
if (record) {
112+
recordId = me.getRecordId(record);
108113
}
109114
}
115+
116+
if (recordId && !selectionModel.isSelected(recordId)) {
117+
selectionModel.select(recordId);
118+
}
110119
}
120+
}
111121

112122
/**
113123
* Override this method to get different item-markups
@@ -122,7 +132,7 @@ class CountryGallery extends Gallery {
122132
fN = Util.formatNumber,
123133
table = firstChild.cn[1];
124134

125-
vdomItem.id = me.getItemVnodeId(me.store.getKey(record));
135+
vdomItem.id = me.getItemVnodeId(me.getRecordId(record));
126136

127137
vdomItem.cn[0].style.height = me.itemHeight + 'px';
128138

@@ -162,7 +172,8 @@ class CountryGallery extends Gallery {
162172
* @param {String[]} items
163173
*/
164174
onSelect(items) {
165-
this.country = items[0] || null;
175+
let record = items[0] ? this.store.get(items[0]) : null;
176+
this.country = record ? record.country : null;
166177
}
167178

168179
/**

‎apps/covid/view/country/Helix.mjs‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -114,18 +114,29 @@ class CountryHelix extends Helix {
114114
}
115115

116116
/**
117-
* Triggered after the country config got changed
117+
* Triggered after the country config got changed.
118+
* We need to distinguish between manual view-based selections (like keynav or clicks) and
119+
* programmatic selections that originate from other widgets (like the combobox or hash change).
120+
* If the change is external, we update the selection and trigger the rotation animation ("fireworks").
118121
* @param {String|null} value
119122
* @param {String|null} oldValue
120123
* @protected
121124
*/
122125
afterSetCountry(value, oldValue) {
123126
if (oldValue !== undefined) {
124127
let me = this,
125-
selectionModel = me.selectionModel;
128+
selectionModel = me.selectionModel,
129+
recordId = value;
126130

127-
if (value && !selectionModel.isSelected(value)) {
128-
selectionModel.select(value, false);
131+
if (me.useInternalId && value) {
132+
let record = me.store.get(value);
133+
if (record) {
134+
recordId = me.getRecordId(record);
135+
}
136+
}
137+
138+
if (recordId && !selectionModel.isSelected(recordId)) {
139+
selectionModel.select(recordId, false);
129140
me.onKeyDownSpace(null);
130141
}
131142
}
@@ -143,7 +154,7 @@ class CountryHelix extends Helix {
143154
fN = Util.formatNumber,
144155
table = firstChild.cn[1];
145156

146-
vdomItem.id = me.getItemVnodeId(me.store.getKey(record));
157+
vdomItem.id = me.getItemVnodeId(me.getRecordId(record));
147158

148159
firstChild.cn[0].cn[0].src = Util.getCountryFlagUrl(record.country);
149160
firstChild.cn[0].cn[1].text = record.country;
@@ -189,7 +200,8 @@ class CountryHelix extends Helix {
189200
* @param {String[]} items
190201
*/
191202
onSelect(items) {
192-
this.country = items[0] || null;
203+
let record = items[0] ? this.store.get(items[0]) : null;
204+
this.country = record ? record.country : null;
193205
}
194206
}
195207

‎apps/sharedcovid/view/country/Gallery.mjs‎

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -92,22 +92,32 @@ class CountryGallery extends Gallery {
9292
*/
9393
store: CountryStore
9494
}
95-
96-
/**
97-
* Triggered after the country config got changed
98-
* @param {String|null} value
99-
* @param {String|null} oldValue
100-
* @protected
101-
*/
102-
afterSetCountry(value, oldValue) {
103-
if (oldValue !== undefined) {
104-
let selectionModel = this.selectionModel;
105-
106-
if (value && !selectionModel.isSelected(value)) {
107-
selectionModel.select(value, false);
95+
/**
96+
* Triggered after the country config got changed.
97+
* We need to distinguish between manual view-based selections (like keynav or clicks) and
98+
* programmatic selections that originate from other widgets (like the combobox or hash change).
99+
* @param {String|null} value
100+
* @param {String|null} oldValue
101+
* @protected
102+
*/
103+
afterSetCountry(value, oldValue) {
104+
if (oldValue !== undefined) {
105+
let me = this,
106+
selectionModel = me.selectionModel,
107+
recordId = value;
108+
109+
if (me.useInternalId && value) {
110+
let record = me.store.get(value);
111+
if (record) {
112+
recordId = me.getRecordId(record);
108113
}
109114
}
115+
116+
if (recordId && !selectionModel.isSelected(recordId)) {
117+
selectionModel.select(recordId);
118+
}
110119
}
120+
}
111121

112122
/**
113123
* Override this method to get different item-markups
@@ -122,7 +132,7 @@ class CountryGallery extends Gallery {
122132
fN = Util.formatNumber,
123133
table = firstChild.cn[1];
124134

125-
vdomItem.id = me.getItemVnodeId(me.store.getKey(record));
135+
vdomItem.id = me.getItemVnodeId(me.getRecordId(record));
126136

127137
vdomItem.cn[0].style.height = me.itemHeight + 'px';
128138

@@ -162,7 +172,8 @@ class CountryGallery extends Gallery {
162172
* @param {String[]} items
163173
*/
164174
onSelect(items) {
165-
this.country = items[0] || null;
175+
let record = items[0] ? this.store.get(items[0]) : null;
176+
this.country = record ? record.country : null;
166177
}
167178

168179
/**

‎apps/sharedcovid/view/country/Helix.mjs‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -114,18 +114,29 @@ class CountryHelix extends Helix {
114114
}
115115

116116
/**
117-
* Triggered after the country config got changed
117+
* Triggered after the country config got changed.
118+
* We need to distinguish between manual view-based selections (like keynav or clicks) and
119+
* programmatic selections that originate from other widgets (like the combobox or hash change).
120+
* If the change is external, we update the selection and trigger the rotation animation ("fireworks").
118121
* @param {String|null} value
119122
* @param {String|null} oldValue
120123
* @protected
121124
*/
122125
afterSetCountry(value, oldValue) {
123126
if (oldValue !== undefined) {
124127
let me = this,
125-
selectionModel = me.selectionModel;
128+
selectionModel = me.selectionModel,
129+
recordId = value;
126130

127-
if (value && !selectionModel.isSelected(value)) {
128-
selectionModel.select(value, false);
131+
if (me.useInternalId && value) {
132+
let record = me.store.get(value);
133+
if (record) {
134+
recordId = me.getRecordId(record);
135+
}
136+
}
137+
138+
if (recordId && !selectionModel.isSelected(recordId)) {
139+
selectionModel.select(recordId, false);
129140
me.onKeyDownSpace(null);
130141
}
131142
}
@@ -143,7 +154,7 @@ class CountryHelix extends Helix {
143154
fN = Util.formatNumber,
144155
table = firstChild.cn[1];
145156

146-
vdomItem.id = me.getItemVnodeId(me.store.getKey(record));
157+
vdomItem.id = me.getItemVnodeId(me.getRecordId(record));
147158

148159
firstChild.cn[0].cn[0].src = Util.getCountryFlagUrl(record.country);
149160
firstChild.cn[0].cn[1].text = record.country;
@@ -189,7 +200,8 @@ class CountryHelix extends Helix {
189200
* @param {String[]} items
190201
*/
191202
onSelect(items) {
192-
this.country = items[0] || null;
203+
let record = items[0] ? this.store.get(items[0]) : null;
204+
this.country = record ? record.country : null;
193205
}
194206
}
195207

0 commit comments

Comments
 (0)