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.

Sign up for GitHub

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

Jump to bottom

Support the following style params #791

Merged
merged 2 commits into from
Jul 18, 2016
Merged

Support the following style params #791

merged 2 commits into from
Jul 18, 2016

Conversation

tallytalwar
Copy link
Member

@tallytalwar tallytalwar commented Jun 14, 2016 •
edited
Loading

  • text:order (not used in ES!!, but documentation mentions this to be a required property for all style
    types)
  • text:visible
  • outline:visible

@tallytalwar
Copy link
Member Author

tallytalwar commented Jun 14, 2016 •
edited
Loading

@blair1618 do you want to have a quick look at this? Can get away with the release after this gets merged.

@tallytalwar
Copy link
Member Author

I am unsure of text:order parameter. But the documentation says all styles need to have order property.

@matteblair
Copy link
Member

These parameters will be parsed now, but they aren't used in any style-building process so this doesn't actually change any behavior (though it does silence some warnings, I suppose). If we are adding keys for these parameters, we should also add their respective behaviors.

@karimnaaji
Copy link
Member

In here text:order would indeed be parsed, but would be a no-op on ES because of how label works, we can probably be more specific and output a warning.

@tallytalwar
Copy link
Member Author

Aiiiiii... I am too eager to push the release out that I missed a part of
the work here :(. Will update.
On Jun 14, 2016 12:03 PM, "Karim Naaji" notifications@github.com wrote:

In here text:order would indeed be parsed, but would be a no-op on ES
because of how label works, we can probably be more specific and output a
warning.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#791 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAWAwQuNZWFwLG0Xfbge_FKasUAozlO-ks5qLtCwgaJpZM4I1emi
.

@tallytalwar
Copy link
Member Author

Updated, along with a warning for usage of text:order.

matteblair
matteblair reviewed Jun 14, 2016
View reviewed changes
core/src/scene/styleParam.cpp
case StyleParamKey::collide:
case StyleParamKey::text_collide:
if (_value == "true") { return true; }
if (_value == "false") { return false; }
LOGW("Bool value required for capitalized/visible. Using Default.");
break;
case StyleParamKey::text_order:
LOGW("text:order style paramater is not used in ES.");
Copy link
Member

Choose a reason for hiding this comment

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

Spelling/style: please revise to "text:order parameter is ignored." or omit.

@tallytalwar
Copy link
Member Author

Updated.

@tallytalwar tallytalwar mentioned this pull request Jun 14, 2016
Closed
@tallytalwar
Copy link
Member Author

Marking this for review, since we have come to a conclusion with respect to visible params' usage.

@tallytalwar
Support the following style params
7bce142 
- text:order (not used!!, but documentation mentions this to be a required property for all style
types)
- text:visible
- outline:visible
@tallytalwar
Apply outline_visible and text_visible params
fec5e25 
- also warn for usage of text:order
@tallytalwar
Copy link
Member Author

Updated and rebased with master.

@matteblair
Copy link
Member

LGTM. Merging!

@matteblair matteblair merged commit 86bc680 into master Jul 18, 2016
@karimnaaji karimnaaji deleted the moreStyleParams branch July 20, 2016 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
ready for review small-fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tallytalwar @matteblair @karimnaaji