Group cart #188
Group cart #188
Conversation
| @@ -0,0 +1,150 @@ | |||
| <div class="details"> | |||
There was a problem hiding this comment.
eventually it'd be good to replace the magic numbers in this template file, too
There was a problem hiding this comment.
I'll convert this to an issue.
| <div | ||
| class="molecule" | ||
| > | ||
| <!-- <inplace class="molecule_name">--> |
There was a problem hiding this comment.
will this commented-out code be useful in the future?
There was a problem hiding this comment.
I think it depends on whether we want to enable renaming after molecules are submitted to personal/group carts. Currently there is no endpoint to support that, so the commented-out input field won't be useful in itself. But it shouldn't be difficult to add that backend functionality if it becomes necessary. I'll delete this section for now.
| </div> | ||
|
|
||
| <div class="molecule_content" [class.vertical]="vertical"> | ||
| <!-- todo: add molecule preview here --> |
There was a problem hiding this comment.
has this "todo" comment already been addressed with the div.preview below it?
There was a problem hiding this comment.
Looks like it. I'll remove this comment.
| <div | ||
| class="value" | ||
| [innerHtml]=" | ||
| getPredictedProperty( |
There was a problem hiding this comment.
this might never be a real performance problem, but we should consider adjusting the template a bit so getPredictedProperty() isn't being called every change-detection cycle
There was a problem hiding this comment.
What's a good way to approach this? It seems like a good use case for Angular Signals, specifically computed(). Or maybe RxJS combineLatest?
There was a problem hiding this comment.
I'll create an issue for this.
| <div class="button" (click)="updateMoleculeLabel(nameInput.value)"> | ||
| Update | ||
| </div> | ||
| <!-- <div class="button" (click)="updateMoleculeLabel(nameInput.value)">--> |
There was a problem hiding this comment.
will this commented-out code be useful in the future?
There was a problem hiding this comment.
I don't think so. I'll delete this.
| import { EnvironmentService } from './environment.service'; | ||
| import { BlockSetId } from './block.service'; | ||
|
|
||
| export const GUEST_USER = 'guest'; |
There was a problem hiding this comment.
it could be nice if the frontend and backend shared a single source of truth on this name, but no need to do anything about it now
There was a problem hiding this comment.
or does the backend even have a concept of a guest user? if not, you can ignore my comment above!
There was a problem hiding this comment.
The guest user is a fake user that only exists on the frontend. It's created here:
digital-molecule-maker/src/app/services/user.service.ts
Lines 49 to 56 in 3134afc
| id: '1_g2u2woll', | ||
| title: 'Structure-Function', | ||
| description: | ||
| 'Can can change the block information from structure to function view at any time. Click on an individual block to switch between function and structure modes', |
There was a problem hiding this comment.
the other descriptions end with a punctuation mark, so we should probably have one here, too, for consistency
| id: '1_9qo362mt', | ||
| title: 'Zoom', | ||
| description: | ||
| 'You can Zoom your in-process molecule, or click to reset to default view.', |
There was a problem hiding this comment.
I'd vote for leaving "zoom" lowercase
| { | ||
| id: '1_z7evpegt', | ||
| title: 'Filter', | ||
| description: 'Filter by block type using tags', |
There was a problem hiding this comment.
I suggest a period at the end
| { | ||
| id: '1_qbzk4ge6', | ||
| title: 'Build and Properties', | ||
| description: |
There was a problem hiding this comment.
typo here: "(our touch)"
typo here: "it's properties"
| id: '1_m6hmfn2h', | ||
| title: 'Graph', | ||
| description: | ||
| 'When you start making block choices, the graph will update with how many molecules are still possible.', |
There was a problem hiding this comment.
suggested rewording: "the graph will update to show the molecules that are still possible."
| { id: 'build_002', class: 'wide' }, | ||
| { id: 'build_003', class: 'wide' }, | ||
| { id: 'build_004', class: 'wide' }, | ||
| // { id: 'workspace', class: '' }, |
There was a problem hiding this comment.
will these commented-out lines be useful in the future?
There was a problem hiding this comment.
Probably not. I'll delete these lines and the associated template sections.
| "lambdaMaxShift": 385, | ||
| "molecularWeight": 403.5 | ||
| "molec |
There was a problem hiding this comment.
this is probably a dumb question on my part, but do we need the chemicalFormula and smiles fields?
There was a problem hiding this comment.
I think we do need these fields. It’s just that we don’t have the correct formula and SMILES for Color Wheel yet. The original code simply concatenates the SMILES strings, which I believe produces invalid results. David Friday’s script requires reactive functional groups to be present, but we don’t have those in the Color Wheel JSON file.
No description provided.