-
Notifications
You must be signed in to change notification settings - Fork 54
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
Encapsulate the row rendering logic in a separate component #27
Conversation
hey @sherrif10 do you mind fixing the build errors |
Fixed linters cc @jabahum |
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.
Thanks @sherrif10 just one more tweak before we can get this in
<TableCell> | ||
<span>{formatDate(parseDate(dateActivated))}</span> | ||
</TableCell> |
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.
add class to make the cell display date on a single line
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.
@sherrif10 and @pirupius hopefully the actions button will also be in a single line
@sherrif10 to avoid lint errors just make sure to run yarn verify before pushing to confirm everything is fine |
.single-line-date { | ||
display: inline-block; | ||
white-space: nowrap; | ||
overflow: hidden; | ||
text-overflow: ellipsis; | ||
} | ||
|
||
.single-line-action { | ||
display: inline-block; | ||
white-space: nowrap; | ||
overflow: hidden; | ||
text-overflow: ellipsis; |
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 is duplication. You can change the class name to something more generic and reuse it in both scenarios
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.
Thanks for reviews
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.
@sherrif10 please update the PR description with the latest screenshot
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.
@pirupius i added the lasted screen shot.
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.
@sherrif10 your screenshot shows the date breaking to the next line. The date should be on a single line.
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.
@sherrif10 You can use something like this in the class you added
white-space: nowrap;
.single-line-content { | ||
display: inline-block; | ||
white-space: nowrap; |
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.
Hey @pirupius . this should really work , not sure if there other styles that conflicting 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.
Hey @pirupius , Updated the screen shot.
src/queue-list/laboratory-queue.scss
Outdated
} | ||
|
||
.table { |
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's change this to something more intentional and also not to conflict with other usages. What do u think of changing it to .single-line-display
@@ -111,7 +111,9 @@ const LaboratoryPatientList: React.FC<LaboratoryPatientListProps> = () => { | |||
date: { | |||
content: ( | |||
<> | |||
<span>{formatDate(parseDate(entry.dateActivated))}</span> | |||
<span className={styles.table}> |
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.
Same change will apply here
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.
Resolved
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.
Thanks @sherrif10
Requirements
Summary
Screenshots
Related Issue
Other
I introduced CustomTableRow Component which Promotes a cleaner and more modular structure by encapsulating the row rendering logic in a CustomTableRow component.
Also i did some code refactory makes it easier to manage the changes in the structure of the table.
Though the initial approach also has an advantage in the way that it makes each table cell individually defined which makes customization little easy than this approach which is more generic. Let me know your suggestions. cc @denniskigen @pirupius @jabahum . Thanks.