Skip to content
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

A page for individual element flowcells #876

Open
wants to merge 45 commits into
base: master
Choose a base branch
from

Conversation

alneberg
Copy link
Member

@alneberg alneberg commented Sep 24, 2024

OK, this became a bit bigger than I first anticipated.

I'll save an update of the flowcells list to another PR.

@alneberg alneberg marked this pull request as ready for review October 17, 2024 14:06
@ssjunnebo
Copy link
Contributor

OK, this became a bit bigger than I first anticipated.

I'm not in a position to complain 😅

<th class="sort searchable" data-sort="start_date">Start date</th>
<th class="sort searchable" data-sort="run_name">Run name</th>
<th class="sort searchable" data-sort="run_type">Run type</th>
<th class="sort searchable" data-sort="side">Side</th>
<th class="sort searchable" data-sort="cycles">Cycles</th>
Copy link
Member

@aanil aanil Oct 18, 2024

Choose a reason for hiding this comment

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

Was not adding the searchable class to Outcome deliberate(line 32)? It doesn't have the search box at present.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. There's no search boxes anywhere neither on this page nor on the Illumina one. So maybe just remove the searchable tag?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah sorry I looked at the wrong page. I'll fix the list of flowcells in another PR.

Comment on lines +121 to +127
} else {
barcode_str += "N/A"
}

if (sample.hasOwnProperty("I2") && sample["I2"] !== ""){
barcode_str += "+" + sample["I2"];
}
Copy link
Member

@aanil aanil Oct 18, 2024

Choose a reason for hiding this comment

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

I guess there will never be a case where I2 exists without I1.

Copy link
Member Author

@alneberg alneberg Oct 21, 2024

Choose a reason for hiding this comment

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

Not sure what you mean. I guess it will look a bit weird in that case with N/A+GATATCGA for example. But I guess if it's rare that could be ok? Or did you mean something else?

Copy link
Contributor

@ssjunnebo ssjunnebo left a comment

Choose a reason for hiding this comment

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

Nice! Added a couple of comments. Also:

  • The list of undetermined indexes is very long. Maybe 20 like for Illumina is enough?
  • Project name is missing in the table in the Lane Stats (Pre-demultiplex) tab

run_dir/design/element_flowcells.html Show resolved Hide resolved
run_dir/design/element_flowcells.html Show resolved Hide resolved
@alneberg
Copy link
Member Author

@ssjunnebo, The list of undetermined indices can now be expanded 5 at a time. Hope that's ok?

Also, the project name should now be possible to show in the pre-demultiplex tab. However, the project ID for the example flowcell is wrong (lims-stage) so it's not visible on that one.

@ssjunnebo
Copy link
Contributor

@ssjunnebo, The list of undetermined indices can now be expanded 5 at a time. Hope that's ok?

Very fancy! I'm happy with that 😄

Also, the project name should now be possible to show in the pre-demultiplex tab. However, the project ID for the example flowcell is wrong (lims-stage) so it's not visible on that one.

Would it be better to show the project ID instead on N/A if this is the case?

@alneberg
Copy link
Member Author

Also, the project name should now be possible to show in the pre-demultiplex tab. However, the project ID for the example flowcell is wrong (lims-stage) so it's not visible on that one.

Would it be better to show the project ID instead on N/A if this is the case?

Good idea! And it was easy as well!

@aanil
Copy link
Member

aanil commented Oct 22, 2024

The list of undetermined indices can now be expanded 5 at a time. Hope that's ok?

Should this be applied on the Show undetermined table on the Lane Stats(Pre demultiplex) tab as well?

Copy link
Member

@aanil aanil left a comment

Choose a reason for hiding this comment

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

Looks good!

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

Successfully merging this pull request may close these issues.

3 participants