OBPIH-6989 ability to sort delivery note by product on stock movement details page#5166
Conversation
github-actions
bot
added
type: feature
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5166 +/- ##
============================================
- Coverage 8.15% 8.12% -0.04%
+ Complexity 946 939 -7
============================================
Files 638 638
Lines 43205 43212 +7
Branches 10502 10502
============================================
- Hits 3525 3511 -14
- Misses 39140 39164 +24
+ Partials 540 537 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| link={downloadOption.uri} | ||
| buttonTitle={downloadOption.name} | ||
| {...document} | ||
| key={idx} |
There was a problem hiding this comment.
I'm not sure if we even need this line. It works fine with it, but I'm not certain what it's actually doing (I just copied it from the below button). Maybe someone with more frontend experience can answer
There was a problem hiding this comment.
| <g:if test="${document.downloadOptions}"> | ||
| <span class="action-menu"> | ||
| <button class="action-btn button"> | ||
| <warehouse:message code="default.button.download.label"/> |
There was a problem hiding this comment.
I know that you are not the person who added this line, but we are using g:message
| <warehouse:message code="default.button.download.label"/> | |
| <g:message code="default.button.download.label"/> |
There was a problem hiding this comment.
I'm going to leave it as is since warehouse:message is used everywhere in the file and I want to refactor as little as possible right now, but that's good to know
| link={downloadOption.uri} | ||
| buttonTitle={downloadOption.name} | ||
| {...document} | ||
| key={idx} |
There was a problem hiding this comment.
index shouldn't be a key. Moreover - this map is nested, hence you will have the same key for those elements.
There was a problem hiding this comment.
why is it bad to make it the index? I don't think this array is mutable (and so indexes don't change). It just uses data returned from the backend. (But yes I'll fix it to better handle the nested map.)
I suppose I could change the key to be the document/download name
There was a problem hiding this comment.
- the key should be unique. since you have nested
.map, theidxwill be the same for each download option (you useidxfrom the nested map)
There was a problem hiding this comment.
yes I understand that part and I'll fix that to make the index actually unique.
I watched the video and it makes sense (thanks for sharing). I was wondering if it's fine to use index in this case since the array shouldn't be changing, but actually I'm not certain about it anymore since there's the ability to add/remove documents from this page. I don't know what the behaviour is in that situation.
But that's a pre-existing concern since this was already using index as the id. And there isn't a better option right now in the payload IMO. The only other possible field to use is name (which is translated and not guaranteed to be unique). I think I'm going to make it work with index as it was before and add a TODO to fix it to provide a proper unique id.
4 participants
✨ Description of Change
Link to GitHub issue or Jira ticket: https://pihemr.atlassian.net/browse/OBPIH-6989
Description: Change the delivery note download dropdown on the documents tab of the stock movement details page to support sorting by order index and by product name.
📷 Screenshots & Recordings (optional)
A recording I made for Kelsey that demos the behaviour: https://jam.dev/c/780f9f32-57af-4aa6-be16-63e5bb81147d