Skip to content

Custom Reproducibility Checklists , Attributes and their Unit testing. - #445

Open
dibyanshu-pal-kushwaha wants to merge 5 commits into
StatTag:masterfrom
dibyanshu-pal-kushwaha:osre26
Open

Custom Reproducibility Checklists , Attributes and their Unit testing.#445
dibyanshu-pal-kushwaha wants to merge 5 commits into
StatTag:masterfrom
dibyanshu-pal-kushwaha:osre26

Conversation

@dibyanshu-pal-kushwaha

Copy link
Copy Markdown
Contributor
Untitled 2 3 4 5 6 7 8

Reproducibility checklists

  • Checked for duplicate checklists.
  • All other discussed features.

@lrasmus lrasmus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I started reviewing the custom attribute and found something with the use of localStorage I wanted to report right away. I'll keep reviewing the rest of the code for the checklist items as you work on this.

setNewAttributeName('');
}

const handleDeleteCustomAttribute = (attributeId) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we want to give the user a pop-up warning and confirmation here. Something like:

Are you sure you want to delete this custom attribute?
Note: This will delete the custom attribute for all assets in this project.

I want the user to be sure they know it's not deleting it for just a single attribute - that if they proceed, it disappears for everything.

>
<DialogTitle className={styles.dialogTitle}>Custom Attributes</DialogTitle>
<DialogContent className={styles.dialogContent}>
<label className={styles.formLabel}>Name of Attributes</label>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very minor - remove "s" on "Attributes" ("Name of Attribute")

<label className={styles.formLabel}>Name of Attributes</label>
<TextField
autoFocus
placeholder='Attributes like "Experimental"'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change to:

'e.g., "Experimental"'

*/
function loadCustomAttributes(projectId) {
try {
const stored = localStorage.getItem(`statwrap_custom_attrs_${projectId}`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unfortunately I don't think the localStorage is going to work as a solution, and we will need to rethink this.

I was able to test this out where I access the same project directory from two different systems. In the first system, I ran StatWrap and defined a new custom attribute:

Image

I then opened StatWrap on the second system and accessed the project. You can see the custom attribute isn't available:

Image

Instead of using localStorage, can you look at writing this to the StatWrap project? This could be a new file in the .statwrap folder specific to the customization. That way, anyone who accesses the project will have access to the configuration details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants