Highlight changed hashes for Deps columns #2029
Conversation
mattseddon
left a comment
mattseddon
left a comment
There was a problem hiding this comment.
Functionality looks good:
| const value = shortenForLabel(hash) | ||
| acc[path] = value | ||
|
|
||
| if (value && branch && branch?.deps?.[path] !== value) { |
There was a problem hiding this comment.
[Q] Is this definitely accessing the right property in the branch?
There was a problem hiding this comment.
I can see from
export interface DepColumns {
[path: string]: string | ValueWithChanges
}
that you've got mixed types. It would be a good idea to standardise the type now as we are moving towards Studio style tooltips for deps:
| styles.td, | ||
| cell.isPlaceholder && styles.groupPlaceholder, | ||
| { | ||
| [styles.workspaceChange]: |
There was a problem hiding this comment.
[I] Name needs to be updated. Personally I think it would be ok to have two different (currently duplicate) styles. One for workspace changes and another for dep changes against HEAD.
| } | ||
|
|
||
| const stringValue = String(value) | ||
| const rawValue = (value as ValueWithChanges).value ?? value |
There was a problem hiding this comment.
[I] Would be good to have a couple of helper functions for
const rawValue = (value as ValueWithChanges).value ?? value
const cellhasChanges = (cell.value as ValueWithChanges)?.changes
so that we can keep them together and have the concept all in one place (easier to extend/reason about later)
wolmir
force-pushed
the
data-pipeline-column-changes-highlight
branch
from
6bfaa05 to
5521a15
Compare
| for (const [path, { hash }] of Object.entries(columns)) { | ||
| acc[path] = shortenForLabel(hash) | ||
| const value = shortenForLabel(hash) | ||
| acc[path] = { |
There was a problem hiding this comment.
You could move this below the if clause and add a continue in the if clause. That way you wouldn't assign to acc[path] twice.
| } | ||
|
|
||
| export interface ValueWithChanges { | ||
| value: string | number |
There was a problem hiding this comment.
Could this ever be anything else than a string or number? I think we have a similar interface somewhere and we also have string[], number[], boolean, boolean[] in there.
| {...cell.getCellProps({ | ||
| className: cx( | ||
| styles.td, | ||
| cell.isPlaceholder && styles.groupPlaceholder, |
There was a problem hiding this comment.
It doesn't really matter, but since we have a block with conditionals just below, this could be moved there as [styles.groupPlaceholder]: cell.isPlaceholder
|
Code Climate has analyzed commit cbb0834 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 96.7% (0.0% change). View more on Code Climate. |
mattseddon
added
🏠 housekeeping
product
|
@wolmir please remember to add one of the three labels to your PRs or they won't make it into the release changelog. The labels are :product: or :housekeeping: or 🐛 |
3 participants
This PR will highlight the dependency cells with changes in relation to HEAD.
Demo:
Screen.Recording.2022-07-18.at.12.21.46.mov