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

separate legacyShareStyle from isLegacyShare #22615

Merged
merged 1 commit into from May 25, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 6 additions & 5 deletions apps/src/StudioApp.js
Expand Up @@ -260,7 +260,7 @@ StudioApp.prototype.init = function (config) {
config.getCode = this.getCode.bind(this);
copyrightStrings = config.copyrightStrings;

if (config.isLegacyShare && config.hideSource) {
if (config.legacyShareStyle && config.hideSource) {
$("body").addClass("legacy-share-view");
if (dom.isMobile()) {
$("body").addClass("legacy-share-view-mobile");
Expand Down Expand Up @@ -310,6 +310,7 @@ StudioApp.prototype.init = function (config) {
level: config.level,
noHowItWorks: config.noHowItWorks,
isLegacyShare: config.isLegacyShare,
legacyShareStyle: config.legacyShareStyle,
wireframeShare: config.wireframeShare,
});
}
Expand Down Expand Up @@ -467,7 +468,7 @@ StudioApp.prototype.init = function (config) {
}.bind(this));
}

if (config.isLegacyShare && config.hideSource) {
if (config.legacyShareStyle && config.hideSource) {
this.setupLegacyShareView();
}

Expand Down Expand Up @@ -1917,7 +1918,7 @@ StudioApp.prototype.handleHideSource_ = function (options) {

// Chrome-less share page.
if (this.share) {
if (options.isLegacyShare || options.wireframeShare) {
if (options.legacyShareStyle || options.wireframeShare) {
document.body.style.backgroundColor = '#202B34';
if (options.level.iframeEmbed) {
// so help me god.
Expand All @@ -1927,7 +1928,7 @@ StudioApp.prototype.handleHideSource_ = function (options) {

$('.header-wrapper').hide();
var vizColumn = document.getElementById('visualizationColumn');
if (dom.isMobile() && (options.isLegacyShare || !dom.isIPad())) {
if (dom.isMobile() && (options.legacyShareStyle || !dom.isIPad())) {
$(vizColumn).addClass('chromelessShare');
} else {
$(vizColumn).addClass('wireframeShare');
Expand Down Expand Up @@ -1955,7 +1956,7 @@ StudioApp.prototype.handleHideSource_ = function (options) {
ReactDOM.render(React.createElement(WireframeButtons, {
channelId: project.getCurrentId(),
appType: project.getStandaloneApp(),
isLegacyShare: options.isLegacyShare,
isLegacyShare: !!options.isLegacyShare,
}), div);
}
}
Expand Down
1 change: 1 addition & 0 deletions apps/src/code-studio/appOptions.js
Expand Up @@ -31,6 +31,7 @@
* @property {boolean} isExternalProjectLevel
* @property {boolean} isChannelBacked
* @property {boolean} isLegacyShare
* @property {boolean} legacyShareStyle
* @property {PostMileStoneMode} postMilestoneMode
* @property {string} puzzleRatingsUrl
* @property {string} authoredHintViewRequestsUrl
Expand Down
2 changes: 1 addition & 1 deletion apps/src/code-studio/initApp/project.js
Expand Up @@ -523,7 +523,7 @@ var projects = module.exports = {
this.sourceHandler.setInitialLevelSource(currentSources.source);
this.showMinimalProjectHeader();
}
} else if (appOptions.isLegacyShare && this.getStandaloneApp()) {
} else if (appOptions.legacyShareStyle && this.getStandaloneApp()) {
Copy link
Contributor Author

@cpirich cpirich May 24, 2018

Choose a reason for hiding this comment

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

This is the only one I'm not certain on. Are we trying to determine if we're a /c share URL here? We're outside of the this.isProjectLevel() check. One reason to choose legacyShareStyle is that it is true in all of the places where we previously had set isLegacyShare to true - so I'm not changing any behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment on showMinimalProjectHeader() reads

// Minimal project header for viewing channel shares and legacy /c/ share pages.

which I think means you've got it right - we want the minimal header in both cases.

this.setName('Untitled Project');
this.showMinimalProjectHeader();
}
Expand Down
6 changes: 1 addition & 5 deletions apps/src/templates/WireframeButtons.jsx
Expand Up @@ -106,11 +106,7 @@ let WireframeButtons = React.createClass({
},

renderNewProjectButton: function () {
// Unfortunately, isLegacyShare currently has the wrong value (is true for non-legacy artist
// and playlab). So instead, we check the first letter (after the /) in the path name of the
// url, as legacy shares all start with /c
// var isLegacyShare = this.props.isLegacyShare;
var isLegacyShare = window.location.pathname[1] === 'c';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code had noticed that something was wrong (the comment says isLegacyShare currently has the wrong value) - but the fix was hacky and narrow in scope.

const { isLegacyShare } = this.props;
var appTypeAndLegacy = this.props.appType + (isLegacyShare ? '_legacy' : '');
var url = APP_TYPE_TO_NEW_PROJECT_URL[appTypeAndLegacy];
if (url) {
Expand Down
2 changes: 1 addition & 1 deletion apps/src/turtle/artist.js
Expand Up @@ -327,7 +327,7 @@ Artist.prototype.init = function (config) {
this.studioApp_.setPageConstants(config, appSpecificConstants);

var iconPath = '/blockly/media/turtle/' +
(config.isLegacyShare && config.hideSource ? 'icons_white.png' : 'icons.png');
(config.legacyShareStyle && config.hideSource ? 'icons_white.png' : 'icons.png');
var visualizationColumn = (
<ArtistVisualizationColumn
showFinishButton={!!config.level.freePlay && !config.level.isProjectLevel}
Expand Down
8 changes: 4 additions & 4 deletions dashboard/app/controllers/level_sources_controller.rb
Expand Up @@ -25,8 +25,8 @@ def show
else
# sharing
level_view_options(@level_source.level_id, hide_source: true)
view_options(no_header: true, no_footer: true, code_studio_logo: true)
@is_legacy_share = true
view_options(no_header: true, no_footer: true, code_studio_logo: true, is_legacy_share: true)
@legacy_share_style = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this controller, we set both is_legacy_share and legacy_share_style

end

respond_to do |format|
Expand All @@ -38,8 +38,8 @@ def show
def edit
authorize! :read, @level_source
level_view_options(@level_source.level_id, hide_source: false)
view_options(small_footer: true)
@is_legacy_share = true
view_options(small_footer: true, is_legacy_share: true)
@legacy_share_style = true
# currently edit is the same as show...
render "show"
end
Expand Down
3 changes: 1 addition & 2 deletions dashboard/app/controllers/projects_controller.rb
Expand Up @@ -300,7 +300,7 @@ def show
# for sharing pages, the app will display the footer inside the playspace instead
no_footer = sharing
# if the game doesn't own the sharing footer, treat it as a legacy share
@is_legacy_share = sharing && !@game.owns_footer_for_share?
@legacy_share_style = sharing && !@game.owns_footer_for_share?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the projects controller, we only ever set legacy_share_style (in some cases)

view_options(
readonly_workspace: sharing || readonly,
full_width: true,
Expand All @@ -309,7 +309,6 @@ def show
no_footer: no_footer,
code_studio_logo: sharing && !iframe_embed,
no_header: sharing,
is_legacy_share: @is_legacy_share,
small_footer: !no_footer && (@game.uses_small_footer? || @level.enable_scrolling?),
has_i18n: @game.has_i18n?,
game_display_name: data_t("game.name", @game.name)
Expand Down
6 changes: 3 additions & 3 deletions dashboard/app/helpers/levels_helper.rb
Expand Up @@ -382,7 +382,7 @@ def weblab_options
sublevelCallback: @sublevel_callback,
}

if (@game && @game.owns_footer_for_share?) || @is_legacy_share
if (@game && @game.owns_footer_for_share?) || @legacy_share_style
app_options[:copyrightStrings] = build_copyright_strings
end

Expand Down Expand Up @@ -547,7 +547,7 @@ def blockly_options

# User/session-dependent options
app_options[:disableSocialShare] = true if (current_user && current_user.under_13?) || app_options[:embed]
app_options[:isLegacyShare] = true if @is_legacy_share
app_options[:legacyShareStyle] = true if @legacy_share_style
app_options[:isMobile] = true if browser.mobile?
app_options[:labUserId] = lab_user_id if @game == Game.applab || @game == Game.gamelab
if @level.game.use_firebase?
Expand Down Expand Up @@ -586,7 +586,7 @@ def blockly_options
end
app_options[:send_to_phone_url] = send_to_phone_url if app_options[:sendToPhone]

if (@game && @game.owns_footer_for_share?) || @is_legacy_share
if (@game && @game.owns_footer_for_share?) || @legacy_share_style
app_options[:copyrightStrings] = build_copyright_strings
end

Expand Down
1 change: 1 addition & 0 deletions dashboard/app/helpers/view_options_helper.rb
Expand Up @@ -14,6 +14,7 @@ module ViewOptionsHelper
:is_external_project_level,
:is_channel_backed,
:is_legacy_share,
:legacy_share_style,
:is_challenge_level,
:post_milestone_mode,
:is_bonus_level,
Expand Down
2 changes: 1 addition & 1 deletion dashboard/app/views/levels/_blockly.html.haml
@@ -1,4 +1,4 @@
- unless @is_legacy_share
- unless @legacy_share_style
%div{style: 'display: none;'}= render partial: 'levels/reference_area'
- if @level.contained_levels.present?
%div{style: 'display: none;'}= render partial: 'levels/contained_levels'
Expand Down