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

Use snake_case style arguments for ui.notify() #930

Merged
merged 1 commit into from
May 26, 2023

Conversation

CatamountJack
Copy link
Contributor

From @rodja's comment in #928 as a potential change for 1.3...

Draft pull request to change arguments in notify.py to use snake_case style names (and convert to Quasar option names where appropriate).

Also added multi_line as an argument since there is a specific example for multi-line notifications in the documentation, renamed the **kwargs argument to **_kwargs so it is excluded from options (just for clarity), and updated documentation to new argument names.

Not sure if this is the best approach, just putting it out there for consideration.

@CatamountJack
Copy link
Contributor Author

On second thought, while renaming kwargs made for a simple solution, it makes the argument name non-standard and is a solution to problem that doesn't really exist.

@falkoschindler
Copy link
Contributor

@CatamountJack Thanks for the pull request!

I finally found some time to look into it. As you suggested, I changed _kwargs back to kwargs. I guess this is equally readable and a bit more standard naming convention. Besides that I just changed the code layout a bit, but that's certainly a matter of taste.

Just one question: What is the purpose of multi_line, actually? I just can't see any difference between setting it True or False. I'm thinking about removing it from the parameter list and from the demo. But maybe I'm missing something.

@falkoschindler falkoschindler added this to the 1.3.0 milestone May 25, 2023
@CatamountJack
Copy link
Contributor Author

The demo already had an example using the Quasar-style multiLine parameter, so I just added a corresponding Python-style multi_line argument. I figured if there was an example for it, there ought to be an appropriately named argument for it.

As to why it doesn't work.... it appears Quasar notifications will wrap to new lines if the text is long enough anyway. To get multi-line text with line-breaks (which is what I think the multi-line notification demo was actually trying to demonstrate), it's the style-sheet { white-space: pre-line; } that was applied that takes care of that. For that purpose, the multiLine argument is actually irrelevant.

But if you also include the closeBtn='OK' argument, you can see the effect - the OK button in the notification moves to a separate line from the text.

    def multiline():
        ui.button('show', on_click=lambda: ui.notify(
            'Lorem ipsum',
            multiLline=False,
            closeBtn='OK',
        ))

vs

    def multiline():
        ui.button('show', on_click=lambda: ui.notify(
            'Lorem ipsum',
            multiLline=True,
            closeBtn='OK',
        ))

@falkoschindler falkoschindler marked this pull request as ready for review May 26, 2023 06:34
@falkoschindler falkoschindler changed the base branch from main to v1.3 May 26, 2023 07:24
@falkoschindler falkoschindler merged commit 69e7880 into zauberzeug:v1.3 May 26, 2023
@falkoschindler
Copy link
Contributor

@CatamountJack Thanks for the example! Indeed, there is a difference.

But I messed up the merge. Sorry! I needed to merge into v1.3 instead of main, but the rebase couldn't be done after I accidentally merged into v1.3 locally. But now all your commits are in v1.3. Only the association with this PR got lost.

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