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.

Sign up for GitHub

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

Jump to bottom

feat: add "isModalForced" to ModuleOptions #78

Merged
merged 14 commits into from
Apr 11, 2023
Merged

feat: add "isModalForced" to ModuleOptions #78

merged 14 commits into from
Apr 11, 2023

Conversation

yoshito-maeoka
Copy link
Contributor

@yoshito-maeoka yoshito-maeoka commented Mar 26, 2023 •
edited by dargmuesli
Loading

📚 Description

this commit allow to force to display Cookie Modal.
that means to force users to choose any options for cookie setting.

📝 Checklist

  • All commits follow the Conventional Commit format
  • The PR's title follows the Conventional Commit format

dargmuesli
dargmuesli reviewed Mar 26, 2023
View reviewed changes
Copy link
Owner

@dargmuesli dargmuesli left a comment

Choose a reason for hiding this comment

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

Setting the new option to true, the modal is shown on every page load, which I do not expect.

@yoshito-maeoka
Copy link
Contributor Author

thank you for your testing, and sorry, that was not also my expectation.
I fixed it.

@dargmuesli
Copy link
Owner

dargmuesli commented Mar 28, 2023 •
edited
Loading

Thank you. Please ensure all commits follow the Conventional Commit format: https://www.conventionalcommits.org/en/v1.0.0/
Also, master needs to be merged into this branch again, I suppose, to make the build go green 🍏

@yoshito-maeoka
Copy link
Contributor Author

I did not understand the Error above on ubuntu. Is there anything I can do for that?

@dargmuesli
Copy link
Owner

dargmuesli commented Mar 30, 2023 •
edited
Loading

I merged master into your branch.
There is a linter error and a warning that need to be resolved. You can see them running pnpm lint in your fork and even under: https://github.com/dargmuesli/nuxt-cookie-control/pull/78/files.

@yoshito-maeoka
Copy link
Contributor Author

I couldn't figure out, what should I do.

pnpm lint gives following error, even I merged latest master branch under dargmuesli/nuxt-cookie-control .
and linter config, package import, etc. are out of my CR. Does anybody have Any Hints?

Oops! Something went wrong! :(

ESLint: 8.36.0

ESLint couldn't find the config "@nuxtjs/eslint-config-typescript" to extend from. Please check that the name of the config is correct.

The config "@nuxtjs/eslint-config-typescript" was referenced from the config file in "/Users/yoshito/work/nuxt-cookie-control/.eslintrc".

If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.

 ELIFECYCLE  Command failed with exit code 2.

@dargmuesli
Copy link
Owner

Have you run pnpm install beforehand? This project's dependencies change regularly.

@yoshito-maeoka
Copy link
Contributor Author

yoshito-maeoka commented Mar 31, 2023 •
edited
Loading

yes, but it does not help to fix...
at least on my laptop I get still same error.

@yoshito-maeoka
Copy link
Contributor Author

got it.
I placed wrong directory this project, and that caused error and can not figured out until now.
but all things go fine now.
thank you for your time to check!

dargmuesli
dargmuesli reviewed Apr 2, 2023
View reviewed changes
Copy link
Owner

@dargmuesli dargmuesli left a comment

Choose a reason for hiding this comment

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

When the user rejects all cookies, the modal is shown on the next page load. Is this intended?

@yoshito-maeoka
Copy link
Contributor Author

actually yes. because necessary cookies should be allowed by user.
but let me know how it should be, if you don't intend this behaviour.

@dargmuesli
Copy link
Owner

dargmuesli commented Apr 2, 2023 •
edited
Loading

Well, if it's possible to hide the modal and use the page by pressing "decline all"... then it does not make sense to me to later show the modal again. You get my idea?

@yoshito-maeoka
Copy link
Contributor Author

you are right. thus I hide close button, if isForceModal is true.
I pushed a fix for 'decline all' button to hide.

@dargmuesli dargmuesli self-requested a review April 10, 2023 18:08
@dargmuesli dargmuesli changed the title feat: add "isForcedModal" to ModuleOptions feat: add "isModalForced" to ModuleOptions Apr 11, 2023
dargmuesli
dargmuesli approved these changes Apr 11, 2023
View reviewed changes
Copy link
Owner

@dargmuesli dargmuesli left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@dargmuesli dargmuesli merged commit 341ac73 into dargmuesli:master Apr 11, 2023
dargmuesli pushed a commit that referenced this pull request Apr 11, 2023
@semantic-release-bot
chore(release): 5.4.0 [skip ci]
1a72d2a 
# [5.4.0](5.3.0...5.4.0) (2023-04-11)

### Features

* add "isModalForced" to ModuleOptions ([#78](#78)) ([341ac73](341ac73))
@dargmuesli
Copy link
Owner

🎉 This PR is included in version 5.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

@dargmuesli dargmuesli dargmuesli approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yoshito-maeoka @dargmuesli @ym-henkelhiedl