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

Display palette method #28

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rouilj
Copy link
Contributor

@rouilj rouilj commented Feb 4, 2023

This addresses #25.

I am not sure if it's the right way to get App and CommandPal to cooperate, but it works.

// Display the instance
c.displayPalette(true)
// Hide the instance
c.displayPalette(false)
// Toggle from display -> hide or hide -> display
c.displayPalette()

John P. rouillard added 5 commits February 4, 2023 16:53
This is a preliminary implementation of a method to control CommandPal
display. It can be called in 3 ways:

   c = CommandPal(...);
   c.displayPalette()      - acts as a toggle
   c.displayPalette(true)  - displays the palette
   c.displayPalette(false) - hides the palette

Passing anything other than true/false will throw an error.

This implementation has a few warnings from the build system. The code
works as intended, but I don't know enough to determine if these
warnings are a problem.

```
> rollup -c

src/main.js → public/build/bundle.js...
(!) Circular dependency
src/main.js -> src/App.svelte -> src/main.js
(!) Mixing named and default exports
https://rollupjs.org/guide/en/#output-exports
The following entry modules are using named and default exports together:
src/main.js

Consumers of your bundle will have to use chunk['default'] to access
their default export, which may not be what you want. Use
`output.exports: 'named'` to disable this warning
```
Created a new file to house the mechanism linking App and
main.js/CommandPal.

This works as the prior commit did but without any errors during
build. This solution occured to me just after the last commit
finished 8-/.
Must have missed this and tested with older version.
Save activeElement when showModal is false indicating it will be opened.
Comment on lines +1 to +11
let displayPaletteMethod; // temp variable for holding function
// generated by App by calling setDisplayPalette

export function storeDisplayPaletteMethod(method) {
displayPaletteMethod = method;
}

export function retrieveDisplayPaletteMethod() {
return displayPaletteMethod;
}

Copy link
Owner

Choose a reason for hiding this comment

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

This is clever, it's basically using the module file scope, as a container to hold the variable which is a function. This is global to the project, but private everywhere else.

To be honest, I think it's a bit convoluted, but I've sat down for a bit and can't think of a better solution 😅 so nice work 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

I'm happy with your solution, as it needs to be shared with main.js which is outside of the svelte context.
We could use some kind of event to pass the function handler or something, but that sounds even worse haha

src/App.svelte Show resolved Hide resolved
@rouilj
Copy link
Contributor Author

rouilj commented Feb 11, 2023

c.dispatch('close') maybe?

or a wrapper for it for opened/closed and to toggle.

@benwinding
Copy link
Owner

c.dispatch('close') maybe?

or a wrapper for it for opened/closed and to toggle.

Yeah that would be nice to be able to send events to the Command Pal instance 🤔 Then we'd have c.subscribe('...') and c.dispatch(...)

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