Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Modern theme for single file attachment #225

Merged
merged 5 commits into from
Dec 1, 2020

Conversation

Blackbaud-AlexKingman
Copy link
Contributor

No description provided.

@blackbaud-ado
Copy link
Member

@codecov
Copy link

codecov bot commented Nov 20, 2020

Codecov Report

Merging #225 (64cba88) into master (6b3bd07) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #225   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           29        29           
  Lines          869       882   +13     
  Branches       162       168    +6     
=========================================
+ Hits           869       882   +13     
Impacted Files Coverage Δ
...file-attachment/file-attachment-label.component.ts 100.00% <100.00%> (ø)
...dules/file-attachment/file-attachment.component.ts 100.00% <100.00%> (ø)

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 6b3bd07...64cba88. Read the comment docs.

@Blackbaud-AdamFunderburk
Copy link
Contributor

I'm not exactly sure exactly what's happening, but there's something weird about resizing the image thumbnail. Best guess is that if the image is smaller than the expected thumbnail size, the width is increased but the height is not, wrecking the aspect ratio.
image

@Blackbaud-AlexKingman
Copy link
Contributor Author

I'm not exactly sure exactly what's happening, but there's something weird about resizing the image thumbnail. Best guess is that if the image is smaller than the expected thumbnail size, the width is increased but the height is not, wrecking the aspect ratio.

Good find. I've added a fix for this.

@blackbaud-ado
Copy link
Member

skyuxconfig.json Outdated
@@ -34,7 +34,8 @@
},
"app": {
"styles": [
"@skyux/docs-tools/css/docs-tools.css"
"@skyux/docs-tools/css/docs-tools.css",
"src/app/visual-styles.scss"

Choose a reason for hiding this comment

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

This should only be added to the skyuxconfig-e2e.json file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent here was to let the visual demos be a bit more legible for devs doing manual testing. Wouldn't we want that in the skyuxconfig.json as well? Or should we just be putting those styles in the visual component itself?

Choose a reason for hiding this comment

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

This stylesheet will "leak" into the docs pages as well. It might create confusion for future engineers who think the visual-styles.scss file only affects the visual tests. Maybe create a component stylesheet for the visual tests instead? Or, create a new global stylesheet that's named in such a way that it can be used for the "production" pages, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points. I moved it to a component-specific style sheet.

@blackbaud-ado
Copy link
Member

@blackbaud-ado
Copy link
Member

@Blackbaud-AlexKingman Blackbaud-AlexKingman merged commit 092a570 into master Dec 1, 2020
@Blackbaud-AlexKingman Blackbaud-AlexKingman deleted the modern-single-file-attachment branch December 1, 2020 16:55
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.

4 participants