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(ui): custom elevation for posts #158

Merged
merged 5 commits into from
Jan 22, 2017
Merged

feat(ui): custom elevation for posts #158

merged 5 commits into from
Jan 22, 2017

Conversation

simonsmh
Copy link
Contributor

@simonsmh simonsmh commented Jan 19, 2017

Contributing rules

  • Fork the repo and create your branch from canary. Then be sure to put the canary branch as the target for your pull request.
  • Please be sure to follow the contributing guidelines

What kind of change does this PR introduce? (check one with "x")

  • Bug fix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

Does this PR introduce a breaking change? (check one with "x")

  • Yes
  • No

image
Fix #54
Reference: cards-section
to review

@ghost
Copy link

ghost commented Jan 19, 2017

Your pull request doesn't follow our guidelines. Please fix the following:

  • Pull request description must include a screenshot (?)
  • Pull request description must include verification steps (?)
  • Pull request title must be in imperative, present tense (e.g. "add", "fix", "change") (?)

Click here for details.

Thank you! 🙏

This comment was made by GitMagic – Magically enforcing your contribution guidelines.

@ghost
Copy link

ghost commented Jan 19, 2017

This pull request still violates some of our guidelines:

  • Pull request description must include verification steps (?)
  • Pull request title must be in imperative, present tense (e.g. "add", "fix", "change") (?)

Click here for details.

@simonsmh simonsmh changed the title fix(layout): elevation for posts #54 fix: layout elevation for posts #54 Jan 19, 2017
@ghost
Copy link

ghost commented Jan 19, 2017

This pull request still violates some of our guidelines:

  • Pull request description must include verification steps (?)
  • Pull request title must have one of the following prefixes: "feat(*): ", "fix(*): ", "docs(*): ", "style(*): ", "refactor(*): ", "test(*): ", "chore(*): " (?)

Click here for details.

@simonsmh simonsmh changed the title fix: layout elevation for posts #54 fix(layout): elevation for posts #54 Jan 19, 2017
@ghost
Copy link

ghost commented Jan 19, 2017

This pull request still violates some of our guidelines:

  • Pull request description must include verification steps (?)
  • Pull request title must be in imperative, present tense (e.g. "add", "fix", "change") (?)

Click here for details.

@simonsmh simonsmh changed the title fix(layout): elevation for posts #54 fix(layout): fix elevation for posts #54 Jan 19, 2017
@ghost
Copy link

ghost commented Jan 19, 2017

This pull request still violates some of our guidelines:

  • Pull request description must include verification steps (?)

Click here for details.

@simonsmh simonsmh changed the title fix(layout): fix elevation for posts #54 fix(layout): fix elevation for posts Jan 19, 2017
@ghost
Copy link

ghost commented Jan 19, 2017

Thank you, the title and description now looks good! :bowtie:

@simonsmh
Copy link
Contributor Author

simonsmh commented Jan 19, 2017

烦透了!!

@cubesky
Copy link
Collaborator

cubesky commented Jan 19, 2017

@simonsmh 我第一次用的时候跟他奋斗了2个小时

@simonsmh
Copy link
Contributor Author

done done done

@cubesky
Copy link
Collaborator

cubesky commented Jan 19, 2017

@simonsmh 恭喜,第一次遇到这个确实挺难过的

Copy link
Owner

@iblh iblh left a comment

Choose a reason for hiding this comment

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

I haven't decided whether to change this elevation.
It's a preference, so at least we should add a configuration.

@simonsmh
Copy link
Contributor Author

@viosey
Please read https://material.google.com/material-design/elevation-shadows.html#elevation-shadows-elevation-android

Don't.
Without a shadow, nothing indicates that the floating action button is separate from the background surfaces.

如果这算是material design,那么就应当采用card应有的形式。Google mdl的template只是个sample,项目可以自己修改,不用完全照搬啊。
此外,post有相关的elevation设置,与主页的设置不符,造成视觉不适,以及logo区域圆角矩形CSS与card CSS设置亦不相同,造成视觉冲突,此pr也一同修复。

@iblh
Copy link
Owner

iblh commented Jan 19, 2017

@simonsmh 这套 scheme 叫做 paradox 的原意就是表明并非纯粹的 Material Design,而 nexus scheme 才遵守 Material Design。
如果要修改 elevation 的话需要以自定义功能的形式,不能写死。

@simonsmh
Copy link
Contributor Author

好的,不过大概明天晚上才能改了
今天还得写作业(唉

@simonsmh
Copy link
Contributor Author

done

@iblh
Copy link
Owner

iblh commented Jan 20, 2017

呃,可能我没有说清楚,就是配置的话不要使用开关的形式,而是自定义高度,以数值为参数

@simonsmh
Copy link
Contributor Author

simonsmh commented Jan 20, 2017 via email

@Halyul
Copy link
Contributor

Halyul commented Jan 20, 2017

分明是一只大学汪

@simonsmh
Copy link
Contributor Author

是这种样子的嘛?

@iblh
Copy link
Owner

iblh commented Jan 20, 2017

差不多了,我再修改一下

@@ -552,7 +552,7 @@ a {
min-height: 400px;
flex-direction: column;
align-items: stretch;
border-radius: 5px;
border-radius: 2px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Please fix tab (use 4 tabs)

@@ -1,4 +1,4 @@
<div class="post_entry-module mdl-card mdl-cell mdl-cell--12-col fade out">
<div class="post_entry-module mdl-card <% if(theme.card_elevation) { %>mdl-shadow--<%= theme.card_elevation %>dp<% } %> mdl-cell mdl-cell--12-col fade out">
<!-- Post_entry Header -->
<% if(post.thumbnail == undefined) { %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Please use <% if(!post.thumbnail) { %>

@@ -1287,7 +1287,7 @@ a {
#scheme-Isolation .post_entry-module {
margin-bottom: 24px;
width: 100%;
border-radius: 5px;
border-radius: 2px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Please fix tab (use 4 tabs)

@@ -1812,7 +1812,7 @@ a {

#scheme-Isolation .material-post .mdl-card{
width: 100%;
border-radius: 5px;
border-radius: 2px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Please fix tab (use 4 tabs)

@@ -1,4 +1,4 @@
<div class="post_entry-module mdl-card mdl-cell mdl-cell--12-col fade out">
<div class="post_entry-module mdl-card <% if(theme.card_elevation) { %>mdl-shadow--<%= theme.card_elevation %>dp<% } %> mdl-cell mdl-cell--12-col fade out">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use <% if(theme.card_elevation) { %><%= 'mdl-shadow--' + theme.card_elevation + 'dp' %><% } %> for more readability

@@ -1,5 +1,5 @@
<!-- Blog info -->
<div class="mdl-card something-else mdl-cell mdl-cell--8-col mdl-cell--4-col-desktop index-top-block">
<div class="mdl-card <% if(theme.card_elevation) { %>mdl-shadow--<%= theme.card_elevation %>dp<% } %> something-else mdl-cell mdl-cell--8-col mdl-cell--4-col-desktop index-top-block">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use <% if(theme.card_elevation) { %><%= 'mdl-shadow--' + theme.card_elevation + 'dp' %><% } %> for more readability

@@ -1,5 +1,5 @@
<!-- Daily pic -->
<div class="mdl-card daily-pic mdl-cell mdl-cell--8-col index-top-block">
<div class="mdl-card <% if(theme.card_elevation) { %>mdl-shadow--<%= theme.card_elevation %>dp<% } %> daily-pic mdl-cell mdl-cell--8-col index-top-block">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use <% if(theme.card_elevation) { %><%= 'mdl-shadow--' + theme.card_elevation + 'dp' %><% } %> for more readability

@@ -16,7 +16,7 @@
<!-- Hidden Page -->
<div class="material-post_container">
<div class="material-post mdl-grid">
<div class="mdl-card mdl-shadow--4dp mdl-cell mdl-cell--12-col">
<div class="mdl-card <% if(theme.card_elevation) { %>mdl-shadow--<%= theme.card_elevation %>dp<% } %> mdl-cell mdl-cell--12-col">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use <% if(theme.card_elevation) { %><%= 'mdl-shadow--' + theme.card_elevation + 'dp' %><% } %> for more readability

@simonsmh
Copy link
Contributor Author

simonsmh commented Jan 20, 2017 via email

@pidupuis
Copy link
Collaborator

Don't worry about gitmagic for this time, your commit message is good. It was bad config from our part sorry

@pidupuis pidupuis merged commit 339831f into iblh:canary Jan 22, 2017
@iblh iblh changed the title fix(layout): fix elevation for posts feat(ui): custom elevation for posts Jan 28, 2017
@iblh iblh mentioned this pull request Jan 28, 2017
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.

None yet

5 participants