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

Extend Selection Menu to allow text selection #11352

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

marthasharkey
Copy link
Contributor

text-dropdown

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,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@marthasharkey marthasharkey marked this pull request as ready for review October 17, 2024 14:30
Copy link
Contributor

@kazcw kazcw left a comment

Choose a reason for hiding this comment

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

The isTextDropdown mode changes a lot of the behavior of the SelectionMenu; the divergence will only increase in the future--we also have more changes planned for this type of text dropdowns (e.g. filtering mentioned in #11335). As such, instead of adding a new mode to SelectionDropdown, let's create a new component for text selection dropdowns.

Comment on lines +311 to +315
Object.keys(data.value.axis)
.filter((s) => s != 'x')
.map((s) => {
return data.value.axis[s as keyof AxesConfiguration].label
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are using both the keys and the values of data.value.axis, using Object.entries might be simpler, and would avoid the type cast.

@@ -14,6 +14,8 @@ const _props = defineProps<{
title?: string | undefined
labelButton?: boolean
alwaysShowArrow?: boolean
isTextDropdown?: boolean
heading?: string | null
Copy link
Contributor

Choose a reason for hiding this comment

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

Not-present (undefined) or null would have the same meaning--let's avoid a distinction without a difference.

Suggested change
heading?: string | null
heading?: string | undefined

Comment on lines 29 to 35
export interface SelectionMenu {
selected: Ref<string>
title?: string
options: Record<string, SelectionMenuOption>
isTextDropdown?: boolean
heading?: string
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of the isTextDropdown field, let's introduce a separate type:

export interface TextSelectionMenu {
  type: 'textSelectionMenu'
  selected: Ref<string>
  title?: string
  options: Record<string, TextSelectionMenuOption>
  heading?: string
}

The reason for the type field approach is probably not apparent--it fits into my plan for the ToolbarItem types (as we add more and their definitions get more complex):

Eventually, every type will have a type field that must contain a specific value if present; however, the field will be optional for any type that can be recognized by its fields, to allow concise toolbar definitions.

So far, every type has had distinctive enough fields that we haven't needed to make any type fields required (and I haven't got around to defining optional type fields). The new TextSelectionMenu has similar fields to the normal SelectionMenu, so its type field is not optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok! Thanks, I wasn't sure if it was best to adapt the current dropdown or make a new one so I'll separate it now!

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.

2 participants