-
Notifications
You must be signed in to change notification settings - Fork 28
feat(cli): Accept multiple attributes to display #153
Conversation
Haroenv
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.
looks good!
| const attributesToDisplay = (optionsFromArguments.attributesToDisplay || '') | ||
| .split(',') | ||
| .filter(Boolean) | ||
| .map(x => x.trim()); |
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.
technically an attribute could end/start in spaces no? Probably not a real issue
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.
Let's say you run the command:
npx create-instantsearch-app my-app \
--attributes-to-display "name,description, location"
You will get the following attributes: ['name', 'description', ' location']. It will then inject this into your template:
_highlightResult. location.value // → breaks because of the spaceIf you trim the values, you'd get: ['name', 'description', 'location']. This is the reason why we need this check.
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.
yes, knew why there could be valid padding, was just wondering if an attribute could start/end with spaces 🤔
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 can't see any reasons why. Do you?
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.
Now we know that we may have a bug here. Since it's rather an edge case, we can move forward.
| cell.textLabel?.highlightedTextColor = .blue | ||
| cell.textLabel?.highlightedBackgroundColor = .yellow | ||
| cell.textLabel?.highlightedText = SearchResults.highlightResult(hit: hit, path: "name")?.value | ||
| cell.textLabel?.highlightedText = SearchResults.highlightResult(hit: hit, path: "{{attributesToDisplay.[0]}}")?.value |
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'm not sure how to handle multiple attributes UI-wise in Swift.
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.
Let's make that an issue and ask some help from the mobile team #178
| "react": "16.4.1", | ||
| "react-instantsearch-native": "{{libraryVersion}}", | ||
| "react-native": "0.56.0" | ||
| "react-native": "0.55.4" |
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.
React Native 0.56.0 was not compatible with the template because of the Expo SDK v27.
e3b9322 to
d0ffc8d
Compare
BREAKING CHANGE: The option `mainAttribute` has been renamed `attributesToDisplay` and now accepts multiple attributes. Closes #151
d0ffc8d to
f4a1518
Compare
|
Note After releasing this new feature, update the new InstantSearch documentation on the Algolia website: npx create-instantsearch-app ais-ecommerce-demo-app \
--template "React InstantSearch" \
--app-id B1G2GM9NG0 \
--api-key aadef574be1f9252bb48d4ea09b5cfe5 \
--index-name demo_ecommerce \
- --main-attribute name
+ --attributes-to-display name |
…antsearch-app#153) * feat(cli): Accept multiple attributes to display BREAKING CHANGE: The option `mainAttribute` has been renamed `attributesToDisplay` and now accepts multiple attributes. Closes algolia/create-instantsearch-app#151 * docs(readme): Document `attributesToDisplay` * test(e2e): Update tests with `attributesToDisplay` * build(release): Update release-template script with `attributesToDisplay` * feat(templates): Update templates with `attributesToDisplay` * test(snapshots): Update template snapshots * fix(template): Remove unused CSS imports
Changes
mainAttributehas been replaced byattributesToDisplayand now accepts multiple attributes--attributes-to-displayattributesToDisplay(it was not adapting the hits template depending onmainAttribute)Preview
Using as an argument:
Closes #151.