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
UI: [VAULT-21532] Create message #24407
UI: [VAULT-21532] Create message #24407
Changes from all commits
85c6097
56c6846
f358607
d01b98a
d520b65
bd225e2
4512457
27ea7de
6817a4e
b449605
f9fd9d3
bb18706
88ce18b
7c0521d
8030a92
e4aa33d
4798cac
ef59ae6
4214bb2
7b63062
0660e86
5e6ac6e
8bf7609
c1935bf
b6edf77
63475dc
3433239
24c03c8
58d7c38
cb743f7
3938ca1
fc73f5f
1ceb1f2
e3a82d5
2beeefc
62d4f40
49d8caa
24b5aa1
6240747
7e110b3
87d47f8
3c6eddc
6303dbe
29c0193
e56a137
b5d28e6
cdf8f74
0856e8e
9116b97
7d50f5b
2faf60e
cba3c26
ea3919f
286cbd9
6070022
30424e1
9611ec2
3786144
f1279a4
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.
For custom messages, we discussed encoding the string when it's sent to the backend and decoding it when we show the message on the details screen as well as on the authenticated / unauth screens.
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.
Instead of having this component manage setting the model value, could you just pass the attribute and an "onChange" function that passes back the end time as either an empty string or a timestamp? I don't think you'd need a component file in that case 🤔
That way this component acts more as a fancy input, and all of the model updates happen in the
create-and-edit-
formThere 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 did something similar at one point in this PR, but removed it since I thought it was easier to understand this way since you're able to see the logic in the template rather than navigating back to the
create-and-edit-
to see the onChange action. 🤷♀️ I think we'll need a component no matter what since the groupValue and formDateTime tracked properties are initially set in the constructor based on whether an endTime exists. An argument can be made that this could be in thecreate-and-edit-
too, but I feel like this should be role of themessage-expiration-date-form
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'll make note of this as a possible improvement, but don't think it should be a blocker for merging this in the sidebranch. The next ticket I'll work on is the edit so I'll try addressing this there.
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.
Definitely. My suggestion was based on handling the input similar to the pattern of how other form fields work. You definitely have more context here! I find attribute changes are easier to follow if all input components are responsible for is returning the selected/inputed value. In this case, that'd be an ISO timestamp string.