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

SY-1098 Add Ability to Resize Most Schematic Symbols #813

Merged
merged 5 commits into from
Sep 13, 2024

Conversation

pjdotson
Copy link
Contributor

@pjdotson pjdotson commented Sep 6, 2024

Feature Pull Request Template

Key Information

Description

Added ability to resize (almost) any schematic symbols - buttons, switches, values, and setpoints still cannot be resized. Also refactored some code in relevant files.

NOTE: Previous versions of the schematic will have a scale of null, so the options for scaling them will not show up. I don't know a great way to fix this problem, and don't think it matters too much - all it means is that the elements created before this version gets released cannot be scaled. If someone wants to scale that element, they could delete it and replace it with a new one.

Basic Readiness Checklist

  • I have performed a self-review of my code.
  • I have added relevant tests to cover the changes to CI.
  • I have updated user facing documentation accordingly.
  • I have verified code coverage targets are met.

Migrations

  • Console - I have ensured that previous versions of stored data structures are
    properly migrated to new formats.
  • Server - I have ensured that previous versions of stored data structures are
    properly migrated to new formats.

Additional Notes

  • These changes deal with concurrency.
  • These changes affect UI.

Manual QA Additions

  • I have updated the Release Candidate template
    with necessary manual QA steps to test my change.

Breaking Changes

Please list any breaking changes to public or internal packages.

Reviwer Checklist

  • Sufficient test coverage of new additions.
  • Verified all steps in the Readiness checklists.
  • UI changes have been tested.
  • Style and formatting is consistent.
  • Reviewed any relevant changes to concurrent code for safety.
  • Sufficient comments and clarity of code.
  • Any additional documentation necessary for feature is provided and has been reviewed for clarity.

@pjdotson pjdotson added the pluto Pluto UI components related label Sep 6, 2024
@pjdotson pjdotson self-assigned this Sep 6, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0.45662% with 218 lines in your changes missing coverage. Please review.

Project coverage is 46.38%. Comparing base (5b391d8) to head (592e1c9).
Report is 52 commits behind head on rc.

Files with missing lines Patch % Lines
pluto/src/vis/schematic/Symbols.tsx 0.00% 55 Missing ⚠️
pluto/src/vis/light/aether/light.ts 0.00% 50 Missing and 1 partial ⚠️
pluto/src/vis/schematic/Forms.tsx 0.00% 43 Missing ⚠️
pluto/src/vis/schematic/primitives/Primitives.tsx 0.00% 26 Missing ⚠️
pluto/src/vis/light/use.ts 0.00% 22 Missing and 1 partial ⚠️
pluto/src/vis/schematic/registry.ts 0.00% 16 Missing ⚠️
pluto/src/pluto/aether/pluto.ts 0.00% 2 Missing ⚠️
pluto/src/vis/light/aether/index.ts 0.00% 0 Missing and 1 partial ⚠️
pluto/src/vis/light/index.ts 0.00% 0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##               rc     #813      +/-   ##
==========================================
- Coverage   46.48%   46.38%   -0.11%     
==========================================
  Files        1083     1090       +7     
  Lines       67600    67833     +233     
  Branches     3494     3514      +20     
==========================================
+ Hits        31424    31464      +40     
- Misses      35129    35316     +187     
- Partials     1047     1053       +6     
Flag Coverage Δ
pluto 35.48% <0.45%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pjdotson pjdotson changed the title SY-1098 Add Ability to Resize any Schematic Symbol SY-1098 Add Ability to Resize Most Schematic Symbols Sep 7, 2024
@emilbon99
Copy link
Contributor

This generally looks ok. I think we need to make some other changes for this feature to be nice though. Label positions look strange when elements are larger. Take this rotary mixer for example. It's probably pretty common to want to make this thing pretty large. It looks strange to fill in the entire mixer when it is turned on. We should probably make it so you hook up a switch to turn the mixer on and off, then we can make it possible to hide and show the controls states on it, so then it becomes more of a 'static' element.

Also, if you can scale the element, you should also have the option to make the stroke width of the symbol larger or smaller.

Screenshot 2024-09-07 at 8 16 08 PM

@emilbon99
Copy link
Contributor

Screenshot 2024-09-07 at 8 17 52 PM

Just looks kinda strange to have this massive, white rotary mixer on the screen

@emilbon99
Copy link
Contributor

Even though you would probably want to have a rotary mixer that big on your P&ID

Copy link
Contributor

@emilbon99 emilbon99 left a comment

Choose a reason for hiding this comment

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

Approved, although I think we need to spend more time thinking about the larger implications of making sure this feature has a nice UX

@pjdotson pjdotson merged commit 9ba77f0 into rc Sep 13, 2024
34 checks passed
@pjdotson pjdotson deleted the sy-1098-add-ability-to-resize-any-schematic-symbol branch September 13, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pluto Pluto UI components related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants