Skip to content
This repository was archived by the owner on Feb 6, 2024. It is now read-only.

Conversation

@christiankozalla
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Feature / Enhancement

Other information

Issue Number: #1064

Dismiss Low Contrast Warning in app-slide-warning and app-slide-warning-info

I added a new Button to the dialog that informs the user about low contrast on their slides (in app-slide-warning-info.tsx).
The Button closes the dialog and emits and event "deactivateContrastWarning", which is listened to in app-slide-warning.tsx

In app-slide-warning.tsx I added a State property "checkLowContrast" which is a boolean and defaults to true
When "deactivateContrastWarning" is caught in app-slide-warning.tsx, this.checkLowContrast will be set to false

I did not implement something in "settings > customization" yet, to activate contrast warnings again.

Questions

I only tested manually, if the button works. After deactivation, the warning doesn't show up anymore on additional slides. But, when I reloaded the window, the state property "checkLowContrast" appears to be reset to its default (true). So the deactivation is not persistent.

I am opening this PR to get early feedback from you, David, to make sure I am on the right track. (I have no prior experience with Stencil)

Thank you for your feedback!

@peterpeterparker
Copy link
Contributor

Awesome Christian 👍

I suggest the following improvement to the PR: instead of the usage of a local state and an event, we can use a global state management and persist the value in indexed db on each changes.

That's something which is already in place, therefore we can follow the same pattern.

We can add a new state / property to the settings.store.ts, for example contrastWarning and add an onChange to save the value in idb, such as a key deckdeckgo_settings_constrast_warning.

Then, instead of the event, the store state can be use settingsStore.state.constrastWarning.

Agree? Would you like to try to make this improvement?

Let me know if you have questions.

@christiankozalla
Copy link
Contributor Author

Thanks, David, for your detailed description how to implement that. That's what I've been hoping for!

Of course, I'll make this improvement and get back to you if I have questions! 👍

@peterpeterparker
Copy link
Contributor

Awesome!

@christiankozalla
Copy link
Contributor Author

Hi David,
I added three commits to the PR yesterday, which

  • implement the "Deactivate Warnings" Button in Popover
  • add "contrastWarning" property to settingsStore,
  • update the settings.service to load property from indexed db
  • add a new row in Settings > Customization

I had to update i18n aswell. I managed to write content in both german and english, but I used a translator for spanish. So spanish might be incorrect..

Is there anything not working / still missing?

Best regards and have a nice week 👍
Christian

@peterpeterparker
Copy link
Contributor

I will try to review it tomorrow but, from what I see quickly, sounds cool, thanks Christian 👍

@peterpeterparker
Copy link
Contributor

Just added the small question about boolean but, beside that, all cool!

@peterpeterparker peterpeterparker merged commit 118f0fe into deckgo:main Jun 8, 2021
@peterpeterparker peterpeterparker linked an issue Jun 8, 2021 that may be closed by this pull request
9 tasks
@peterpeterparker
Copy link
Contributor

Awesome Christian, thx a lot for the PR!

Do you want to add the option to the "customization" settings page? I don't mind doing it, as you rather like.

@christiankozalla
Copy link
Contributor Author

It was a pleasure! Thank you :)

I believe, this PR already includes the option for the "cuztomization" settings page.

In 162da86, I added two ion-select-options with values "on" and "off" corresponding to true and false

One thing I wondered while testing the select-options: How can I navigate from the "customization" settings page back to my slide, which I just started with? Did I oversee something? Or could I maybe implement that?

@peterpeterparker
Copy link
Contributor

damn me you are right @christiankozalla it was already there, too much multitasking in the evening today 😅

I just made a small PR #1203 to change the toggle into radio, as we are now using boolean

All good then, awesome!!!!!!! Thx a lot 🙏

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dismiss low contrast warning

2 participants