Skip to content

Conversation

be-smith
Copy link
Contributor

@be-smith be-smith commented Jul 30, 2025

Add XRD insitu block for PR datalab-org/datalab-app-plugin-insitu#67

The new block should look as below with the option to toggle between modes

Screenshot 2025-09-10 at 17 11 54

@be-smith be-smith changed the title Bes/xrd insitu block pre rebase XRD insitu block Jul 30, 2025
Copy link

codecov bot commented Jul 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.05%. Comparing base (e5a78e7) to head (373184f).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1287   +/-   ##
=======================================
  Coverage   80.05%   80.05%           
=======================================
  Files          70       70           
  Lines        4729     4729           
=======================================
  Hits         3786     3786           
  Misses        943      943           
Files with missing lines Coverage Δ
pydatalab/src/pydatalab/apps/echem/blocks.py 76.22% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ml-evs ml-evs force-pushed the bes/xrd_insitu_block_pre_rebase branch from c562212 to 91f3404 Compare July 31, 2025 11:10
Copy link

cypress bot commented Jul 31, 2025

datalab    Run #3853

Run Properties:  status check passed Passed #3853  •  git commit 3d530d4f06 ℹ️: Merge 373184fba24c49057432d6badab39022fc7b36e6 into e5a78e7ec96ceb1fd6a38d5106eb...
Project datalab
Branch Review bes/xrd_insitu_block_pre_rebase
Run status status check passed Passed #3853
Run duration 07m 26s
Commit git commit 3d530d4f06 ℹ️: Merge 373184fba24c49057432d6badab39022fc7b36e6 into e5a78e7ec96ceb1fd6a38d5106eb...
Committer Ben Smith
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 336
View all changes introduced in this branch ↗︎

@be-smith be-smith marked this pull request as ready for review July 31, 2025 14:10
@be-smith be-smith requested a review from Copilot July 31, 2025 14:10
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for XRD insitu data visualization by implementing a new XRDInsituBlock component and registering it as a custom block type. The changes enable users to visualize in-situ X-ray diffraction data alongside temperature or electrochemical data.

  • Add XRDInsituBlock component for XRD insitu data visualization
  • Register the new component in the custom block types registry
  • Enhance UV-Vis insitu block with scan time input functionality

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
webapp/src/resources.js Imports and registers the new XRDInsituBlock component in the custom block types
webapp/src/components/datablocks/XRDInsituBlock.vue Implements the complete XRD insitu visualization component with folder selection and data granularity controls
webapp/src/components/datablocks/UVVisInsituBlock.vue Adds scan time input field to the existing UV-Vis insitu block

}
},
updateBlock() {
this.isLoading = true;
Copy link

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The property 'isLoading' is being set but is not defined in the component's data properties. This will cause the property to be created on the Vue instance without reactivity, which could lead to unexpected behavior.

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re these suggestions. They are common across NMRInsituBlock, UVVis and XRD. I assume this.isLoading should be defined in data instead of this.isUpdating and set to false.

@ml-evs ml-evs force-pushed the bes/xrd_insitu_block_pre_rebase branch from 5e04cde to c7aff64 Compare July 31, 2025 14:23
@be-smith be-smith force-pushed the bes/xrd_insitu_block_pre_rebase branch from fea1280 to 2e86260 Compare August 1, 2025 16:37
@be-smith be-smith force-pushed the bes/xrd_insitu_block_pre_rebase branch from 2e86260 to 913fa95 Compare August 22, 2025 11:08
@be-smith be-smith force-pushed the bes/xrd_insitu_block_pre_rebase branch from 1a664ff to da1b336 Compare September 10, 2025 13:14
Copy link
Contributor

@BenjaminCharmes BenjaminCharmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@ml-evs ml-evs moved this to Todo in merge stack Sep 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

3 participants