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

Design Audit Feedback #96

Merged
merged 16 commits into from
Jun 6, 2024
Merged

Design Audit Feedback #96

merged 16 commits into from
Jun 6, 2024

Conversation

KastanDay
Copy link
Contributor

@KastanDay KastanDay commented Feb 7, 2024

Fixes #72

Addressing feedback from doc: https://docs.google.com/document/d/12JqiffBZD8T-K7Hx0cnQqb-Ymr4Ysu-EoPiFATUNeSU/edit

  • 'enter/return creating new line is confusing for users.' Switched input box to a single line, no newlines allowed. I like it.

@KastanDay KastanDay self-assigned this Feb 7, 2024
@KastanDay
Copy link
Contributor Author

My formatting is different from yours. My one line change caused a major diff because of spacing & formatting rules.

How should I format/lint? Is there a specific setup?

Copy link
Member

@bodom0015 bodom0015 left a comment

Choose a reason for hiding this comment

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

@KastanDay I'm seeing some odd behavior here - it looks like when the job is submitted, we no longer see the loading screen while the job completes

I also see the banner indicating that we're looking at example data (useExample=true), even tho it actually does still evaluate the real job results of my uploaded file - is this change intentional?

Example of what i see when running locally: http://recordit.co/1zuKptaGhq

@KastanDay
Copy link
Contributor Author

@KastanDay I'm seeing some odd behavior here - it looks like when the job is submitted, we no longer see the loading screen while the job completes

I also see the banner indicating that we're looking at example data (useExample=true), even tho it actually does still evaluate the real job results of my uploaded file - is this change intentional?

Example of what i see when running locally: http://recordit.co/1zuKptaGhq

So sorry this was for testing purposes only! Will fix. I hardcoded variables to test the UI.

bodom0015
bodom0015 previously approved these changes Feb 19, 2024
Copy link
Member

@bodom0015 bodom0015 left a comment

Choose a reason for hiding this comment

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

Works as advertised, aside from the small issues mentioned above :)

Thank you, @KastanDay! 🎉

@KastanDay KastanDay dismissed bodom0015’s stale review February 19, 2024 16:48

The merge-base changed after approval.

@bodom0015
Copy link
Member

@KastanDay any updates on this PR? I think that once the merge conflicts / hardcoded variables are fixed, we can get this merged up, unless you had anything else you were trying to target here?

Let me know if I can assist or clarify in any way! Thank you 👍

@KastanDay KastanDay force-pushed the design_audit_feedback branch 2 times, most recently from 9e7f269 to 2d3382e Compare February 27, 2024 18:47
@KastanDay
Copy link
Contributor Author

Hi @bodom0015, I just spent 90 minutes trying to resolve the merge conflicts but I can't get the opening and closing to line up... They look deceptively simple, but no matter what it's all messed up.

Could you try once and see if I'm missing something? Don't spend more than 5 minutes on it -- in that case I'll try again.

@KastanDay KastanDay force-pushed the design_audit_feedback branch from 7ee2ac1 to 2d3382e Compare February 27, 2024 18:55
@KastanDay
Copy link
Contributor Author

Still missing these two columns "Similarity" and "Confidence".

CleanShot 2024-02-27 at 15 10 48

@KastanDay
Copy link
Contributor Author

Note I think my local testing of this function isn't working, so it's tricky testing SMILES sorting...

this._chemScraperService.getSimilaritySortedOrder(this.jobID, smile).subscribe(
https://github.com/moleculemaker/chemscraper-frontend/pull/96/files#diff-8a5fc6884fd2b68103c448423e57e81f8a77b6569f2226c049142cf52a4fb842R492

bodom0015
bodom0015 previously approved these changes Feb 29, 2024
Copy link
Member

@bodom0015 bodom0015 left a comment

Choose a reason for hiding this comment

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

LGTM, aside from the minor issues mentioned above!
Thank you, @KastanDay! 🎉 🎊

You mentioned the Confidence and Similarity columns are missing - is this due to my bad merge? Or are have these features simply not been implemented yet?

@KastanDay
Copy link
Contributor Author

KastanDay commented Mar 20, 2024

@bodom0015
Copy link
Member

Thank you, @KastanDay! 🎉

Tool Selector

I think that "tool selector" here refers to the top-left of the navbar header beneath where it says "TOOL"... I think the subtle change here is to turn that into a dropdown menu allowing you to more easily find links to all of the AlphaSynthesis tools: ChemScraper, CLEAN, MOLLI and a few others

We should be able to copy the same dropdown functionality over to all of the related AlphaSynthesis frontends as well, which all use the same Angular + PrimeNG patterns 👍

Our existing AlphaSynthesis tools:
Screenshot 2024-03-20 at 3 57 54 PM

Job Summary

According to the Figma designs, it looks like "Job Summary" refers to adding a bit more context to the header above the results table:
Screenshot_2024-03-20_at_3_48_15_PM

Copy link
Member

@bodom0015 bodom0015 left a comment

Choose a reason for hiding this comment

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

LGTM, @KastanDay! 👍 I deployed this to ChemScraper frontend staging for testing/review

I can see the MarvinJS modal is fixed and the citation now appears at the bottom 🎉

@KastanDay KastanDay merged commit a16aacc into main Jun 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants