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

add (expandedWidth, borderSize, collapsedWidth) parameters to the sidePanel component #3606

Closed
wants to merge 147 commits into from

Conversation

Sofien-Sellami
Copy link
Contributor

Context

I've made these changes to give the option of changing the sidepanel parameters (expandedWidth, borderSize, collapsedWidth)

Changes & Results

in this example I've expanded the width of the sidePanel
<SidePanel expandedWidth={500} side="right" activeTabIndex={rightPanelDefaultClosed ? null : 0} tabs={rightPanelComponents} servicesManager={servicesManager} />

expanded-sidePanel

Testing

Checklist

PR

  • My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • OS:
  • Node version:
  • Browser:

@netlify
Copy link

netlify bot commented Aug 16, 2023

Deploy Preview for ohif-platform-docs ready!

Name Link
🔨 Latest commit 6723d39
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/652e913f25ef2e0008a5838c
😎 Deploy Preview https://deploy-preview-3606--ohif-platform-docs.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.

@netlify
Copy link

netlify bot commented Aug 16, 2023

Deploy Preview for ohif-dev canceled.

Name Link
🔨 Latest commit 6723d39
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/652e913ffcec860008e7818b

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #3606 (6723d39) into master (8a335bd) will decrease coverage by 0.15%.
Report is 69 commits behind head on master.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3606      +/-   ##
==========================================
- Coverage   46.23%   46.09%   -0.15%     
==========================================
  Files          78       78              
  Lines        1276     1280       +4     
  Branches      312      312              
==========================================
  Hits          590      590              
- Misses        548      552       +4     
  Partials      138      138              
Files Coverage Δ
platform/core/src/utils/downloadCSVReport.js 0.00% <ø> (ø)
...ils/metadataProvider/getPixelSpacingInformation.js 0.00% <0.00%> (ø)
platform/core/src/log.js 14.28% <0.00%> (-85.72%) ⬇️
...services/DicomMetadataStore/createStudyMetadata.js 0.00% <0.00%> (ø)
...ervices/DicomMetadataStore/createSeriesMetadata.js 0.00% <0.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33f1259...6723d39. Read the comment docs.

@sedghi
Copy link
Member

sedghi commented Oct 3, 2023

Sorry for my late response, but can you please update this based on the new side panel?

ohif-bot and others added 25 commits October 5, 2023 15:18
…ata source with Google cloud healthcare implementation (OHIF#3589)
@sedghi
Copy link
Member

sedghi commented Oct 5, 2023

git pull origin master --no-rebase
and solve the conflicts

@sedghi sedghi requested a review from jbocce October 17, 2023 02:23
position={side === 'left' ? 'right' : 'left'}
key={index}
content={`${childComponent.label}`}
<>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see a lot of places like this where the indentation changed and I am not sure why. Could you please fix this?

Copy link
Collaborator

@jbocce jbocce left a comment

Choose a reason for hiding this comment

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

Good change! Thanks!

See my one comment.

@jbocce
Copy link
Collaborator

jbocce commented Oct 17, 2023

Hmmm... the title of the PR says that also borderSize and collapsedWidth can be changed. I do not see those props. I am happy with just changing the expandedWidth for now if that is ok or are you planning on making more changes?

@Sofien-Sellami
Copy link
Contributor Author

Hmmm... the title of the PR says that also borderSize and collapsedWidth can be changed. I do not see those props. I am happy with just changing the expandedWidth for now if that is ok or are you planning on making more changes?

hi @jbocce, I fixed the stuff you asked for, and I don't think I'm going to add the bordersize and collapsedWidth props because I think they need to be fixed.

@jbocce
Copy link
Collaborator

jbocce commented Oct 17, 2023

Hmmm... the title of the PR says that also borderSize and collapsedWidth can be changed. I do not see those props. I am happy with just changing the expandedWidth for now if that is ok or are you planning on making more changes?

hi @jbocce, I fixed the stuff you asked for, and I don't think I'm going to add the bordersize and collapsedWidth props because I think they need to be fixed.

That makes sense and is my preference too.

Copy link
Collaborator

@jbocce jbocce left a comment

Choose a reason for hiding this comment

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

Approved. Thanks again!

@sedghi
Copy link
Member

sedghi commented Oct 17, 2023

@Sofien-Sellami please git pull origin master --no-rebase and resolve conflicts and push again

@Sofien-Sellami
Copy link
Contributor Author

@Sofien-Sellami please git pull origin master --no-rebase and resolve conflicts and push again

HI @sedghi, there is no change

@sedghi
Copy link
Member

sedghi commented Oct 17, 2023

sorry you origin is your fork, so git pull upstream? what ever you are calling it

git remote -v

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.