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

Feat/improve css compatibility #61

Merged
merged 7 commits into from
Jan 28, 2020
Merged

Conversation

matcornic
Copy link
Owner

@matcornic matcornic commented Jan 26, 2020

Great news, a friend of mine has lent me a Windows with a licensed Outlook. So I can fix the Outlook compatibility issue.

So this PR contains code for:

=> Replaces PR #48

Hey @kauffj @loeffel-io @vjeantet @dmuriel @elliots 🙌
Can you test this PR on your side ?

It worked for me on a Windows 10 with latest version of Outlook. I think it's still perfectible, but it does the work.

Edit: the size of the button now adapts to content in default theme.

default.go Outdated Show resolved Hide resolved
default.go Show resolved Hide resolved
default.go Show resolved Hide resolved
flat.go Show resolved Hide resolved
flat.go Show resolved Hide resolved
hermes.go Show resolved Hide resolved
hermes.go Show resolved Hide resolved
hermes.go Show resolved Hide resolved
hermes.go Show resolved Hide resolved
hermes.go Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Jan 26, 2020

Codecov Report

Merging #61 into master will decrease coverage by 0.85%.
The diff coverage is 85.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
- Coverage   98.45%   97.59%   -0.86%     
==========================================
  Files           3        3              
  Lines         907      957      +50     
==========================================
+ Hits          893      934      +41     
- Misses          8       16       +8     
- Partials        6        7       +1
Impacted Files Coverage Δ
default.go 100% <100%> (ø) ⬆️
flat.go 99.55% <100%> (+0.01%) ⬆️
hermes.go 68.18% <43.75%> (-9.6%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45fe330...027481f. Read the comment docs.

@loeffel-io
Copy link

Great news! Thank you very much 🚀

@dmuriel
Copy link

dmuriel commented Jan 27, 2020

Thank you very much. I just tested on Windows Server 2016 with Outlook 2016, and email is displaying correctly, except the button, as you said, "does not adapt to content, but it does the work."

@matcornic
Copy link
Owner Author

@dmuriel I will try this evening to adapt the button width from the length of the text (unless it reaches a maximum width). It should be technically doable, but we will see if it's not such a good idea.

@dmuriel
Copy link

dmuriel commented Jan 27, 2020

@matcornic you can also check this workaround I made for Outlook only, maybe you can use something for the button. It may not be pretty but it worked for me while waiting for a solution.

@kauffj
Copy link

kauffj commented Jan 27, 2020

Asking someone else at @lbryio to look at this. Thank you!

@tzarebczan
Copy link

LGTM, thank you again for looking into and fixing this!

default.go Show resolved Hide resolved
default.go Show resolved Hide resolved
@matcornic
Copy link
Owner Author

matcornic commented Jan 27, 2020

@dmuriel The button width in default theme is now adapting to text content since 027481f

I checked with multiple text sizes and it seems to react nicely. I implemented the same behaviour with both classic/Outlook sections for compatibility purpose. max-width and min-width could have been used, but using only width keeps compatibility requirements quite low.

Let me know what you think about it :)

(last commit before merge 🚀)

@dmuriel
Copy link

dmuriel commented Jan 28, 2020

@matcornic I agree with leaving only with the width attribute. Regarding the button width it seems that now is cropping the text.
"Outlook Web"
"Outlook 2016"

@matcornic
Copy link
Owner Author

@dmuriel weird, I used the trial of Litmus, and the button looks good on Outlook 2016.

image

I will merge as is.

@matcornic matcornic merged commit 9f477cc into master Jan 28, 2020
@matcornic matcornic mentioned this pull request Jan 28, 2020
@lingr7
Copy link

lingr7 commented Sep 26, 2022

image
outlook 2016 windows10 has problem

@lingr7
Copy link

lingr7 commented Sep 26, 2022

theme flat and default has the same problem ,can not show button

@lingr7
Copy link

lingr7 commented Sep 26, 2022

I find a workaround. success show button

DisableCSSInlining: true,
	h := hermes.Hermes{
		// Optional Theme
		// Theme: new(Default)
		Theme:              new(hermes.Flat),
		DisableCSSInlining: true,

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.

8 participants