Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
[NoQA] Feat/44307 card system messages #46281
[NoQA] Feat/44307 card system messages #46281
Changes from all commits
614518e
36661fb
e4284d2
018e939
3137495
f0283e4
e74c3cf
93a771b
b7d963e
9ecee55
2346f07
fcb3cef
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Bug
Wrong system message for
CARD_MISSING_ADDRESS
it says the card will be delivered in few days instead of missing address message c.c @koko57Screen.Recording.2024-07-29.at.4.25.24.PM.mov
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.
That's because you have your address added - if you wouldn't have it a proper message with button to add an address would be displayed. After you add the address it is displayed like this - that's why I added two additional cases to test regardless you have it added or not
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.
You can try to log into a new account that doesn't have address added to test it
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 me give it a try
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.
This occurs with new accounts as well:
Screen.Recording.2024-07-29.at.4.32.02.PM.mov
IMO
noMailingAddress
is always returning false here. can you please take a lookThere 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.
@allgandalf fixed! I forgot that I changed
action?.originalMessage.html
foraction?.actionName
(the proper condition). I added an extra check that will be removed laterThere 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 see that in design doc, we mentioned the name of the admin as well, was it left out on purpose?
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.
we don't have the Admin's name in the message in Figma https://www.figma.com/design/9J4v6CzMVLQbzXMnXC5Izq/%23wave-collect%3A-Workspace-Card-Feeds?node-id=2565-32040&t=p5sUZgAYMIDW3Lsb-0
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.
Odd :) If we are matching with the figma mocks, I'm okay with the current state, thanks for mentioning
This comment was marked as resolved.
Sorry, something went wrong.
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.
Okay i ready the convo #44307 (comment), I'll mark this as resolved
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Okay i ready the convo #44307 (comment), I'll mark this as resolved
This comment was marked as resolved.
Sorry, something went wrong.
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.
Okay i ready the convo #44307 (comment), I'll mark this as resolved