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

fix repeater row top spacing #32

Merged
merged 4 commits into from
Apr 18, 2019
Merged

fix repeater row top spacing #32

merged 4 commits into from
Apr 18, 2019

Conversation

Blackbaud-ToddRoberts
Copy link
Contributor

Addresses #31

@Blackbaud-ToddRoberts
Copy link
Contributor Author

@Blackbaud-AlexKingman this fix seems to not work for the Inline Form repeater example - in that case, sky-repeater-item-left is being displayed as a block rather than inline which is throwing it off. i'm not sure if that is something in the component itself or due to the way the visual demo is constructed.

@Blackbaud-AlexKingman Blackbaud-AlexKingman self-assigned this Apr 16, 2019
Blackbaud-SteveBrush
Blackbaud-SteveBrush reviewed Apr 18, 2019
View reviewed changes
Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

Just a code style comment.

src/app/public/modules/repeater/repeater-item.component.scss Outdated
@@ -23,6 +25,7 @@
.sky-repeater-item-header {
align-items: center;
display: flex;
padding-top: $sky-padding;
Copy link
Member

Choose a reason for hiding this comment

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

All of these entries appear to be missing an extra space character before padding-top:

  display: flex;
 padding-top: $sky-padding;

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't notice this in my IDE, but on closer inspection it looks like they might be tabs instead of spaces! 😲

Copy link
Contributor

Choose a reason for hiding this comment

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

@Blackbaud-ToddRoberts you might want to double check your settings to make sure you're doing 2 spaces per tab.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Blackbaud-SteveBrush this is fixed!

@codecov
Copy link

codecov bot commented Apr 18, 2019 •
edited
Loading

Codecov Report

Merging #32 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #32   +/-   ##
=======================================
  Coverage   99.53%   99.53%           
=======================================
  Files          24       24           
  Lines         431      431           
  Branches       48       48           
=======================================
  Hits          429      429           
  Misses          2        2

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 544d895...f5a6c1b. Read the comment docs.

@Blackbaud-AlexKingman Blackbaud-AlexKingman merged commit 921724f into master Apr 18, 2019
@Blackbaud-AlexKingman Blackbaud-AlexKingman deleted the repeater-row-top-padding branch November 4, 2019 16:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Blackbaud-SteveBrush Blackbaud-SteveBrush Blackbaud-SteveBrush left review comments

@Blackbaud-AlexKingman Blackbaud-AlexKingman Blackbaud-AlexKingman approved these changes

Assignees

@Blackbaud-AlexKingman Blackbaud-AlexKingman

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Blackbaud-ToddRoberts @Blackbaud-SteveBrush @Blackbaud-AlexKingman