Refactor binding list #742
Conversation
| <td className="col-2">{name}</td> | ||
| <td className="col-2">{reason}</td> | ||
| <td className="col-2"> | ||
| <button className="button" onClick={this.openModal}> |
There was a problem hiding this comment.
I think a small button will work better :)
| <button className="button" onClick={this.openModal}> | |
| <button className="button button-small" onClick={this.openModal}> |
| <button className={"button button-primary button-small"} onClick={this.toggleExpand}> | ||
| Expand/Collapse | ||
| return ( | ||
| <tr className="row padding-t-small"> |
There was a problem hiding this comment.
I'm not a big fan of modifying default table behaviour with flex. Is there any reason you had to use row and col classes here?
There was a problem hiding this comment.
not really, the result is almost the same without it, it was just to set specific sizes for the columns
| <a> | ||
| <span | ||
| className="Terminal__Top__Button Terminal__Top__Button--red" | ||
| onClick={this.props.closeModal} |
There was a problem hiding this comment.
Love that red button actually close the modal 😂
| message: string; | ||
| } | ||
|
|
||
| class MessageDetails extends React.Component<IMessageDetailsProps> { |
There was a problem hiding this comment.
Could you make this component Pure?
There was a problem hiding this comment.
agreed offline to make this a Stateless component
|
Thanks @Angelmmiguel for taking a look! Can you do a quick review again? |
|
I think we should hide by default the secret values, can we either show a modal with those values or show them inline with a click of a "show" link next to the secret name? |
|
@prydonius I have applied your suggestions (check the screenshots) |
|
cc/ @Angelmmiguel |
|
Sure! I'm gonna review it @andresmgot. Btw, did you remember the conversation about function vs pure components? React v16.6 comes with a |
| /> | ||
| </a> | ||
| </div> | ||
| <div className="Terminal__Top__Title">Message</div> |
There was a problem hiding this comment.
[Not a blocker] Could you change this title to something more specific? I think Message is to general.
There was a problem hiding this comment.
+1 I would make it a prop so that it can be set by the caller
| /> | ||
| </a> | ||
| </div> | ||
| <div className="Terminal__Top__Title">Message</div> |
There was a problem hiding this comment.
+1 I would make it a prop so that it can be set by the caller
| @@ -0,0 +1,52 @@ | |||
| import * as React from "react"; | |||
There was a problem hiding this comment.
I think this component should be renamed as it is used for the secret and the error message, so a more general name would be better. Perhaps something like TerminalModal makes more sense?
Cool, nice to know, thanks for the heads up! (we are still in react 16.4) I applied the rest of your comments. Thanks for the reviews! |
Co-Authored-By: andresmgot <andres.mgotor@gmail.com>
Fixes #455
This PR refactors how the binding list is represented in the Service Instance View. The summary of the changes are:
Extend/Collapsebutton since it was adding a new row to the table which looked confusing. Instead the "Details" column is now always present. The format of these details has been refactored as well.If these changes are approved we should use the same approach for ordering in
ServiceInstanceCardListand apply the "Show message" button to the service instance message as well.Screenshots:
When clicking in "Show message":
When clicking in the "show" link of the secret a similar modal is displayed:
For the record this is how the table looks without these changes: