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

New data page #254

Merged
merged 1 commit into from Feb 3, 2018
Merged

New data page #254

merged 1 commit into from Feb 3, 2018

Conversation

ar5had
Copy link
Member

@ar5had ar5had commented Feb 1, 2018

data

@ar5had ar5had self-assigned this Feb 1, 2018
@ar5had ar5had added the Web app label Feb 1, 2018
@jankeromnes
Copy link
Member

Wow, this looks great! A few quick comments off hand:

  • please remove the "0 merged". It's wrong and will probably never work since we've changed what the status does
  • please rename "contributions" to "containers"

<h3>Containers</h3>
<p class="numbers">
<span class="number">{{= data.contributions.started in integer}}</span>
<span class="unit">started</span>
Copy link
Member Author

Choose a reason for hiding this comment

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

@jankeromnes I have removed the merged status. I hope this is what you meant.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, perfect. Thank you! 😄

@ar5had
Copy link
Member Author

ar5had commented Feb 1, 2018

#253 needs to be merged before this pr. I will then have to update this pr.

Copy link
Member

@jankeromnes jankeromnes 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 to me, thanks! I approve this change, but please remove the 404 page commit from your branch, e.g. with:

  • git fetch origin
  • git rebase -i origin/master
  • (delete the line with the unrelated 404 page commit)
  • (save the file)

border-bottom: none;
}

.data div:last-child::after {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment with what this represents ? Also, I think .data::after might be better, because right now, it kinda makes an assumption that it has a link with :last-child when it really doesn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

image
^ its use. I know its unnecessry but I like the psuedo separation that it creates between the border.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but as I said, add a comment, it's not explicit. Also, can you just put this on .data::after ?

}

.data div:nth-child(4),
.data div:nth-child(5) {
Copy link
Member

Choose a reason for hiding this comment

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

Is :nth-child(5) used ? Also, can you use .data-topic instead of div ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Edit: I should use nth-f-type instead.


.data .data-topic {
flex: 0 1 50%;
/* background: red; */
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove this leftover

.data-topic .number {
font-size: 18px;
font-weight: 700;
color: #0c9100;
Copy link
Member

Choose a reason for hiding this comment

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

@ar5had
Copy link
Member Author

ar5had commented Feb 3, 2018 via email

@nt1m
Copy link
Member

nt1m commented Feb 3, 2018

@arshdkhn1 Alright, I'm fine with using .data .data-topic:last-child::after then, but please do add a comment saying what this does.

@ar5had ar5had force-pushed the new-data-page branch 3 times, most recently from c3d00c4 to 8e9670c Compare February 3, 2018 11:45
@nt1m nt1m force-pushed the new-data-page branch 3 times, most recently from 1f3087d to 8eb444a Compare February 3, 2018 12:03
@nt1m nt1m dismissed jankeromnes’s stale review February 3, 2018 12:09

Already addressed Jan's comments

@ar5had ar5had merged commit 084df57 into JanitorTechnology:master Feb 3, 2018
@ar5had ar5had deleted the new-data-page branch February 3, 2018 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants