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

Consolidate radiobuttons.js and radio2.js logic #2203

Merged
merged 10 commits into from
Oct 7, 2024
Merged

Consolidate radiobuttons.js and radio2.js logic #2203

merged 10 commits into from
Oct 7, 2024

Conversation

creilly8
Copy link
Contributor

@creilly8 creilly8 commented Oct 7, 2024

Description

Changes:

  1. Consolidated radiobuttons.js and radio2.js logic into typescript.
  2. Deleted radio2.js
  3. New unit tests for make_radios()

Test, everywhere it's implemented:

  1. Matrix in mass -> Click on 3 Genes -> Click on different radio buttons -> Appearance and functionality should be the same as prod.
  2. Maf example -> same as prod.
  3. Variant label in lollipop -> same as prod
  4. Single cell example -> Radio buttons at the top should be the same as master.
  5. AI check example -> Click on CONFIG. -> Radio buttons for pixel should appear and work the same as prod
  6. Bam example -> Click on CONFIG. -> Radio buttons for the reads should appear and work the same as prod
  7. Condition term example -> Edit the first term -> should work the same as prod
  8. Discrete example -> Click on the pill and click on edit. -> Buttons in discrete menu should appear and function the same as prod
  9. New unit tests

Checklist

Check each task that has been performed or verified to be not applicable.

  • Tests: added and/or passed unit and integration tests, or N/A
  • Todos: commented or documented, or N/A
  • Notable Changes: updated release.txt, prefixed a commit message with "fix:" or "feat:", added to an internal tracking document, or N/A

@creilly8 creilly8 changed the title Issue.2176 Consolidate radiobuttons.js and radio2.js logic Oct 7, 2024
@creilly8 creilly8 linked an issue Oct 7, 2024 that may be closed by this pull request
client/dom/radiobutton.ts Outdated Show resolved Hide resolved
client/dom/radiobutton.ts Outdated Show resolved Hide resolved
@creilly8 creilly8 marked this pull request as draft October 7, 2024 19:02
@creilly8 creilly8 marked this pull request as ready for review October 7, 2024 19:13
siosonel
siosonel previously approved these changes Oct 7, 2024
Copy link
Member

@siosonel siosonel left a comment

Choose a reason for hiding this comment

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

Works for me, tested most of the links. I'll leave it to @gavrielm to test gene variant edit UI.

@gavrielm
Copy link
Collaborator

gavrielm commented Oct 7, 2024

geneVariant UI works well. Thanks for the refactor. I noticed the spacing between radio options is noticeably larger compared to that on master. Is this desired?

@gavrielm
Copy link
Collaborator

gavrielm commented Oct 7, 2024

In addition to spacing, the options seem to be misaligned with other text. For example,

cox outcome edit UI (master):
image

cox outcome edit UI (this branch):
image

@creilly8
Copy link
Contributor Author

creilly8 commented Oct 7, 2024

In addition to spacing, the options seem to be misaligned with other text. For example,

cox outcome edit UI (master): image

cox outcome edit UI (this branch): image

@gavrielm, Thanks for pointing that out. The styles were updated to use padding instead of margin. I missed this one. The newest commit fixes the issues you noted.

@gavrielm
Copy link
Collaborator

gavrielm commented Oct 7, 2024

Now spacing between options in Cox edit UI:

image

Is different than spacing between options in geneVariant edit UI:

image

@creilly8
Copy link
Contributor Author

creilly8 commented Oct 7, 2024

Now spacing between options in Cox edit UI:

image

Is different than spacing between options in geneVariant edit UI:

image

@gavrielm, The styling for the time scale toggle removed the margin (in this case padding, set to 0) in master. The same styling removal wasn't repeated for the groupset radios. Do you want them to be the same? If so which one, no padding or the default 5px?

@gavrielm
Copy link
Collaborator

gavrielm commented Oct 7, 2024

I think my point is the spacing between options on this branch:
image

is larger than the spacing between options on master branch:
image

and the code for the geneVariant groupset radios does not specify any styling (so it is just using the default styling specified in make_radios). Sorry, I know this is minor, but the default spacing seems cleaner on master. If anyone disagrees then let me know.

Copy link
Collaborator

@gavrielm gavrielm left a comment

Choose a reason for hiding this comment

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

Thanks. geneVariant and cox outcome edit UIs look good.

@siosonel siosonel merged commit 804f44c into master Oct 7, 2024
3 checks passed
@siosonel siosonel deleted the issue.2176 branch October 7, 2024 22:00
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.

keep only one: dom/radio2 and dom/radiobutton
3 participants