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

[Feature] Disable email template #270

Merged

Conversation

Civolilah
Copy link
Collaborator

@beganovich I've been looking for a solution to disable the MDEditor component from the @uiw/react-md-editor npm package, but I haven't found one. Here's a link to the source where I found it's not yet possible for this component: jrm2k6/react-markdown-editor#56. Please let me know if I'm wrong.

@turbo124 Regarding this situation that I don't have permission to disable this component, I decided not to show this component if the user is not allowed to do so. Please let me know if you agree with this kind of solution if Ben confirms that disabling is not possible.

Here is the screenshot of how it looks without body template:

image

@beganovich
Copy link
Member

beganovich commented Nov 21, 2022

I don't think we should just hide input. My suggestion would be to replace the markdown component with InputField textarea and disable that (only for disabled state, not when writable).

Nonetheless, I think this needs to be updated with the CTA button as we discussed in Slack.

@beganovich beganovich marked this pull request as draft November 21, 2022 09:06
@Civolilah
Copy link
Collaborator Author

@beganovich @turbo124 here is the screenshot how it looks with replaced button.

changePlanEmailTemplate

@Civolilah Civolilah marked this pull request as ready for review November 21, 2022 13:56
@beganovich
Copy link
Member

@turbo124 thoughts?

@turbo124
Copy link
Member

This looks fine

Copy link
Member

@beganovich beganovich left a comment

Choose a reason for hiding this comment

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

Looks good, small changes are needed 👍

/>
) : (
<div className="flex flex-col items-start">
<span className=" text-gray-500" style={{ fontSize: 12 }}>
Copy link
Member

Choose a reason for hiding this comment

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

  • Remove blank space before text-gray-500
  • Use Tailwind representation for text size (probably you'll need text-sm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • Remove blank space before text-gray-500
  • Use Tailwind representation for text size (probably you'll need text-sm.

Fixed

Comment on lines 214 to 216
Email body template can be changed on{' '}
<strong>Enterprise/Pro</strong> plan.
</span>
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be replaced with a translatable string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This needs to be replaced with a translatable string.

Added

@beganovich beganovich marked this pull request as draft November 22, 2022 13:06
@Civolilah
Copy link
Collaborator Author

This looks fine

@turbo124 Regarding missed key for translation, please just include this one. email_template_change = "Email body template can be changed on".

@Civolilah Civolilah marked this pull request as ready for review November 22, 2022 14:28
@beganovich beganovich self-requested a review November 22, 2022 14:56
@beganovich
Copy link
Member

Thanks 👍

@beganovich beganovich merged commit 691f110 into invoiceninja:develop Nov 22, 2022
@Civolilah Civolilah deleted the feature/85-email-template-disable branch December 9, 2022 16:12
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.

3 participants