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

fix menu stretch in long confirmation messages #4892

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

mhsh312
Copy link
Contributor

@mhsh312 mhsh312 commented Apr 12, 2024

Fix cBioPortal/cbioportal#10737

Describe changes proposed in this pull request:

  • Fixed the Confirmation message widening the add chart menu
  • Added max-width css property to SuccessBanner
  • Passed the containerWidth prop to SuccessBanner (similarly how it has been passed to GeneLevelSelection)
  • Updated ISuccessBannerProps accordingly

Because the menu does not have an explicit width and just adjusts according to its children, setting max-width of SuccessBanner to 100% or inherit would not work. So, I passed the current containerWidth to SuccessBanner so that it can have a numerical limit to its width.

If no test is included it should explicitly be mentioned in the PR why there is no test.

It is a simple CSS fix so no extra tests are needed.

Checks

  • Has tests or has a separate issue that describes the types of test that should be created. If no test is included it should explicitly be mentioned in the PR why there is no test.
  • The commit log is comprehensible. It follows 7 rules of great commit messages. For most PRs a single commit should suffice, in some cases multiple topical commits can be useful. During review it is ok to see tiny commits (e.g. Fix reviewer comments), but right before the code gets merged to master or rc branch, any such commits should be squashed since they are useless to the other developers. Definitely avoid merge commits, use rebase instead.
  • Is this PR adding logic based on one or more clinical attributes? If yes, please make sure validation for this attribute is also present in the data validation / data loading layers (in backend repo) and documented in File-Formats Clinical data section!

Any screenshots or GIFs?

image

Notify reviewers

@alisman

Copy link

netlify bot commented Apr 12, 2024

Deploy Preview for cbioportalfrontend ready!

Name Link
🔨 Latest commit c3e1750
🔍 Latest deploy log https://app.netlify.com/sites/cbioportalfrontend/deploys/661d7277c6dd0d0008d110ca
😎 Deploy Preview https://deploy-preview-4892--cbioportalfrontend.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@inodb inodb added the gsoc label Apr 15, 2024
Copy link
Collaborator

@alisman alisman left a comment

Choose a reason for hiding this comment

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

Thank you @mhsh312 !

@alisman
Copy link
Collaborator

alisman commented Apr 15, 2024

@mhsh312 There are some broken e2e tests. Let me know if you have time to figure out what's up or need some help.
https://app.circleci.com/pipelines/github/cBioPortal/cbioportal-frontend/12986/workflows/225289fc-d3ad-40dc-89bf-cb11c23b5cbd/jobs/158792
The instructions for running them are in the README

@mhsh312
Copy link
Contributor Author

mhsh312 commented Apr 15, 2024

@alisman to be honest, I read the failed tests and I have no idea why this happened. Why did a css change break an API test? I could understand this test maybe because it is written to perceive the UI a certain way and it is not finding it but absolutely no idea about the other two. The modified component SuccessBanner also isn't used anywhere else except in the component mentioned in this issue.
Sorry, completely lost on this one.

If you know what the issue is please let me know as I would like to know as well.

Edit: I tried wrapping the component into a div with max-width instead of modifying the actual component but that doesn't pass the test either.

@mhsh312
Copy link
Contributor Author

mhsh312 commented Apr 15, 2024

@alisman it turns out that when I run this test even without the changes I made, check_api_sync.sh test still breaks. So I think that is an issue with some other code unrelated to my change.

@alisman
Copy link
Collaborator

alisman commented Apr 15, 2024 via email

@mhsh312
Copy link
Contributor Author

mhsh312 commented Apr 16, 2024

https://app.circleci.com/pipelines/github/cBioPortal/cbioportal-frontend/12986/workflows/225289fc-d3ad-40dc-89bf-cb11c23b5cbd/jobs/158792

this test passed after I wrapped the SuccessBanner component instead of modifying it in this commit.

Check here:
https://app.circleci.com/pipelines/github/cBioPortal/cbioportal-frontend/12993/workflows/e5e7afaf-9b13-47df-bc7d-5b379d9eb436/jobs/158840

As for the localDB test, i think it fails because now the size of the menu changes in vertical height, so the test needs to be updated.

@inodb inodb requested a review from onursumer April 22, 2024 14:46
Copy link
Member

@onursumer onursumer 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 to me! Not sure if the failing local db tests are related.

@inodb inodb added bug style tweak To indicate the PR does a minor style tweak and removed bug labels Apr 25, 2024
@inodb inodb merged commit a02b26d into cBioPortal:master Apr 25, 2024
12 of 14 checks passed
@inodb inodb changed the title fixed menu stretch in long confirmation messages fix menu stretch in long confirmation messages Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc style tweak To indicate the PR does a minor style tweak
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confirmation message in ad chart interface causes dropdown to widen unpleasantly
4 participants