-
Notifications
You must be signed in to change notification settings - Fork 31
Update TX pages for API parity #92
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
Conversation
awrichar
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.
Approved, but requested a few small tweaks for this or a follow-up
|
I wasn't sure if |
- Remove deprecated fields from UI panels - Remove TX details - Remove links to TX details in Messages view Signed-off-by: Alex Shorsher <alex.shorsher@kaleido.io>
28f7e4e to
be6b6ba
Compare
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
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.
dechdev
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, requested one change
src/core/interfaces.ts
Outdated
| error: string; | ||
| id: string; | ||
| status: string; | ||
| subtype: string; |
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.
error, id, and subtype are all optional (may be undefined)
src/core/interfaces.ts
Outdated
| } | ||
|
|
||
| export interface ITransactionStatus { | ||
| status: string; |
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.
status (here and in the details) is an enum of Pending/Failed/Succeeded, so might be useful to define that
|
@shorsher I think "Status" is correct as the label for Pending/Succeeded/Failed, but I do think it's confusing as the header for the child objects. Perhaps just "Details" for that header? |
|
|
||
| return ( | ||
| <> | ||
| <Typography fontWeight="bold">{t('status')}</Typography> |
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.
Commented on the top-level too... but maybe "Details" is a more appropriate header here
Signed-off-by: Alex Shorsher <alex.shorsher@kaleido.io>

<Slide>and include transaction status objects