Feature/414 clickable table rows #439
Conversation
mbfm
left a comment
mbfm
left a comment
There was a problem hiding this comment.
Hi, thanks for opening another pull request! 🙂
Generally, the changes look reasonable, but it's best to avoid code duplication where reasonably possible. It adds a lot of maintenance overhead when the different code replicas start diverging over time...
| let cluster_id = create_read_slice(cluster_descriptor, |cluster_descriptor| { | ||
| cluster_descriptor.id | ||
| }); | ||
|
|
||
| let cluster_id = create_read_slice(cluster_descriptor, | ||
| |cluster_descriptor| { | ||
| cluster_descriptor.id | ||
| } | ||
| ); | ||
|
|
||
| let cluster_name = create_read_slice(cluster_descriptor, | ||
| |cluster_descriptor| { | ||
| Clone::clone(&cluster_descriptor.name).to_string() | ||
| } | ||
| ); | ||
| let cluster_name = create_read_slice(cluster_descriptor, |cluster_descriptor| { | ||
| Clone::clone(&cluster_descriptor.name).to_string() | ||
| }); |
There was a problem hiding this comment.
If you use a formatter, could you please set it, so that it only formats the lines that you actually modified, please? It makes it difficult to review what you actually changed, is more likely to lead to merge/rebase conflicts and some of the automatic formatting changes are less readable than hand-formatted code...
| let (start_x, start_y) = mousedown_pos.get(); | ||
| let (end_x, end_y) = (e.client_x(), e.client_y()); | ||
| let diff_x = (end_x - start_x) as f64; | ||
| let diff_y = (end_y - start_y) as f64; | ||
| // euclidean distance formula -> distance = sqrt(diff_x² + diff_y²) | ||
| let distance = (diff_x * diff_x + diff_y * diff_y).sqrt(); |
There was a problem hiding this comment.
Could you pull this out into a function in util, which calculates this distance?
Maybe so that it can be called like so:
| let (start_x, start_y) = mousedown_pos.get(); | |
| let (end_x, end_y) = (e.client_x(), e.client_y()); | |
| let diff_x = (end_x - start_x) as f64; | |
| let diff_y = (end_y - start_y) as f64; | |
| // euclidean distance formula -> distance = sqrt(diff_x² + diff_y²) | |
| let distance = (diff_x * diff_x + diff_y * diff_y).sqrt(); | |
| let distance = calculate_distance(mousedown_pos.get(), (e.client_x(), e.client_y())); |
| let diff_y = (end_y - start_y) as f64; | ||
| // euclidean distance formula -> distance = sqrt(diff_x² + diff_y²) | ||
| let distance = (diff_x * diff_x + diff_y * diff_y).sqrt(); | ||
| if distance < 5.0 { |
There was a problem hiding this comment.
Can you pull out this 5.0 into a constant, maybe also into util for now, e.g. MOUSE_DRAG_PIXEL_THRESHOLD.
It would also be good to add a short comment here, to say that we're doing this to fix text selection. Generally, we prefer readable code over comments, but the why cannot easily be expressed in code.
|
hi @mbfm , thanks for the detailed review and the helpful suggestions. I'll get to work on implementing the above suggestions. |
|
I also noticed that it doesn't work in the PeerOverview, seemingly when clicking on the "Configured in Clusters" column, e.g. in these positions: 
Maybe there's a call to |
|
thanks for pointing out, i will try to debug this. |
2 participants
Fixes #414
Clicking on a row in Cluster/Peer Overview now navigates to the respective configurator while taking into account of user selecting texts.