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

Implement full wheel component menu #9029

Conversation

MichaelMauderer
Copy link
Contributor

@MichaelMauderer MichaelMauderer commented Feb 12, 2024

Pull Request Description

Closes #8612.

Peek.2024-02-12.14-11.webm

Important Notes

The ... menu was added at the top of the menu as there is less visual interference with an opened visualisation.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@MichaelMauderer MichaelMauderer added CI: No changelog needed Do not require a changelog entry for this PR. -gui labels Feb 12, 2024
@MichaelMauderer MichaelMauderer self-assigned this Feb 12, 2024
@MichaelMauderer MichaelMauderer marked this pull request as ready for review February 13, 2024 12:38
@MichaelMauderer MichaelMauderer marked this pull request as draft February 13, 2024 13:30
@MichaelMauderer MichaelMauderer marked this pull request as ready for review February 14, 2024 10:31
Comment on lines 10 to 17
isFullMenuVisible: boolean
}>()
const emit = defineEmits<{
'update:isOutputContextOverridden': [isOutputContextOverridden: boolean]
'update:isDocsVisible': [isDocsVisible: boolean]
'update:isVisualizationVisible': [isVisualizationVisible: boolean]
startEditing: []
openFullMenu: []
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the "full menu visible" could be v-model also, similarly to isVisualizaitonVisible, isDocsVisible and all others.

So, not openFullMenu but update:isFullMenuVisible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was actually no longer used, so is just removed now.

width: 100%;
height: 100%;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to repeat properties in .CircularMenuPartial and .CircularMenuFull? @Frizi @somebody1234 any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are not the same thing, they are two different pseudo-elements that contain the graphics for the full and partial background. Only one is used depending on which state the menu is in, and as far as I can tell, they need their respective properties. The only thing shared between them is the fact that they have 100% width and height, and I'm not sure if that's worth factoring out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I just see more than a width: also height, backdrop-filter, background, content and position. Is there a reason we make it using pseudo-elements instead of normal element under the icons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For one, this was how it was implemented in the first place by @somebody1234. And my understanding is, that this allows the structure of the HTML to be simpler and has purely visual things in CSS instead of HTML.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah. for me it's mostly a habit from the dashboard needing backdrop-filter: blur in a lot of places.

that said, it should be possible to use :is(.Foo, .Bar) for the properties that are common between the two.

alternatively you could do .CircularMenu.partial and .CircularMenu.full instead, and use .CircularMenu for the properties in common.
something like this might work:

.CircularMenu {
  &:before {
    /* shared properties here */

    &.partial {
    }
    
    &.full {
    }
  }
}

}

.output-context-overridden {
opacity: 100%;
color: red;
}

.docs-button {
.slot1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add a comment how the slots are placed in the menu. Is Slot1 the bottom most, or leftmost? Are they numbered clockwise, or counterclockwise?

Comment on lines +103 to +111
&:after {
content: '...';
font-size: 15px;
display: block;
text-align: center;
z-index: 10000;
margin-top: -10px;
opacity: 0.3;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Cannot these three dots be just a content of .More? And I don't see any usage of this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a pseudo-element, so it creates a dummy element that is rendered as a child of the element with the .More class. This allows the text to be styled and placed separately.

@MichaelMauderer MichaelMauderer added the CI: Ready to merge This PR is eligible for automatic merge label Feb 15, 2024
@mergify mergify bot merged commit 6746bdc into develop Feb 15, 2024
28 of 29 checks passed
@mergify mergify bot deleted the wip/MichaelMauderer/Full-Wheel-Component-Menu.-Replace-Component-Menu-documentation-icon-with-an-edit-icon branch February 15, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-gui CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Full Wheel Component Menu. Replace Component Menu documentation icon with an edit icon
3 participants