BL-7198 Add Page B.Enterprise mods

[gmartin7]

• Add "BE" to template pages that require enterprise
• make the page Preview a React component

---

This change is 

[andrew-polk]

Gordon and I worked on this some together, but this is the first time I've seen all the code together like this, so I had several comments. Time will tell whether this is still sitting here when he gets back or whether I will "fix" all the things I commented on myself before then...

Reviewed 15 of 15 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @gmartin7)

---

src/BloomBrowserUI/bloomUI.less, line 124 at r1 (raw file):

    user-select: text; // Chrome
}

Moved as-is from toolbox.less to a common location

---

src/BloomBrowserUI/pageChooser/page-chooser.less, line 262 at r1 (raw file):

}

.convertWholeBook {

Surprised to see several selectors changed from id to class.

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 252 at r1 (raw file):

!convertAnywayCheckboxElement

Maybe I missed this when you asked about it before, but...
Isn't this going to prevent us from making changes if the convertAnyway checkbox is not in the dom? I think React will remove it from the dom when it isn't rendered, right?

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 257 at r1 (raw file):

                return;
            }
            const wholeBookCheckboxElement = document.getElementById(

Another (better??) option would be to have the react component pass in the simple boolean it already knows about and doesn't have to look up from the dom and have the other version do this lookup.
Same could be said for the convertAnyway checkbox above.

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 642 at r1 (raw file):

// Copied from react_components/requiresBloomEnterprise.
// If/when this becomes a React component that will no longer be necessary.
export function checkIfEnterpriseAvailable(): EnterpriseEnabledPromise {

I can't see that this function or the following class are used/needed.

---

src/BloomBrowserUI/react_components/requiresBloomEnterprise.tsx, line 41 at r1 (raw file):

        return (
            <div
                className={this.props.className + " requiresBloomEnterprise"}

I know we added this together, but now that we decided not to use it, I think I would prefer we remove it until someone else decides it is useful.

---

src/BloomBrowserUI/react_components/requiresBloomEnterprise.tsx, line 126 at r1 (raw file):

// but the children get to determine how they respond to the context that they read with
// the new useContext hook.
export const NonOpinionatedRequiresBEWrapper: React.FunctionComponent = props => {

I should have said something before, but now that I see it in a PR, I'm not satisfied with this name.
Perhaps the best thing would be to make this a generic name and make the existing one say more explicitly what it does.
But since we are not actually using this yet (after the simplifications made), I'm torn on whether it is worth changing the existing one...

[gmartin7]

Reviewable status: 6 of 18 files reviewed, 6 unresolved discussions (waiting on @andrew-polk)

---

src/BloomBrowserUI/bloomUI.less, line 124 at r1 (raw file):

Previously, andrew-polk wrote…

Moved as-is from toolbox.less to a common location

I added a comment to that effect.

---

src/BloomBrowserUI/pageChooser/page-chooser.less, line 262 at r1 (raw file):

Previously, andrew-polk wrote…

Surprised to see several selectors changed from id to class.

Yes. Switching from plain HTML/Typescript to React involved using the Checkbox React component, which has different ideas about where you can put an id on its innards (combined input and label HTML elements). This necessitated putting a class on an outer div to get the same functionality.

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 252 at r1 (raw file):

Previously, andrew-polk wrote…

!convertAnywayCheckboxElement

Maybe I missed this when you asked about it before, but...
Isn't this going to prevent us from making changes if the convertAnyway checkbox is not in the dom? I think React will remove it from the dom when it isn't rendered, right?

Oops. I got the logic wrong there. Changed.

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 257 at r1 (raw file):

Previously, andrew-polk wrote…

Another (better??) option would be to have the react component pass in the simple boolean it already knows about and doesn't have to look up from the dom and have the other version do this lookup.
Same could be said for the convertAnyway checkbox above.

Reworked this bit some. Ended passing in willLoseData as well as the state of both checkboxes.

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 642 at r1 (raw file):

Previously, andrew-polk wrote…

I can't see that this function or the following class are used/needed.

Done. Deleted.

---

src/BloomBrowserUI/react_components/requiresBloomEnterprise.tsx, line 41 at r1 (raw file):

Previously, andrew-polk wrote…

I know we added this together, but now that we decided not to use it, I think I would prefer we remove it until someone else decides it is useful.

Done.

---

src/BloomBrowserUI/react_components/requiresBloomEnterprise.tsx, line 126 at r1 (raw file):

Previously, andrew-polk wrote…

I should have said something before, but now that I see it in a PR, I'm not satisfied with this name.
Perhaps the best thing would be to make this a generic name and make the existing one say more explicitly what it does.
But since we are not actually using this yet (after the simplifications made), I'm torn on whether it is worth changing the existing one...

Oops! I forgot we didn't need this anymore. Deleted.

[andrew-polk]

Reviewed 12 of 12 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gmartin7)

---

src/BloomBrowserUI/bloomUI.less, line 124 at r1 (raw file):

Previously, gmartin7 (Gordon Martin) wrote…

I added a comment to that effect.

That note was more just for anyone reviewing. Wasn't asking for the comment. But ok.

---

src/BloomBrowserUI/bookEdit/css/bloomDialog.less, line 2 at r2 (raw file):

@import (reference) "../../bookLayout/basePage";
@import "../../bloomWebFonts.less";

Is this a good place to use the reference import?

---

src/BloomBrowserUI/react_components/templatePagePreview.tsx, line 139 at r2 (raw file):

                </div>
            )}
            {enterpriseSubscriptionFault(props.pageIsEnterpriseOnly) && (

I thought we specifically decided not to use the fault method here because RequiresBloomEnterprise already does the check.

[gmartin7]

Reviewable status: 12 of 18 files reviewed, 2 unresolved discussions (waiting on @andrew-polk)

---

src/BloomBrowserUI/bookEdit/css/bloomDialog.less, line 2 at r2 (raw file):

Previously, andrew-polk wrote…

Is this a good place to use the reference import?

I thought so, but it didn't work. The UI Font stack didn't get used when I used (reference).

---

src/BloomBrowserUI/react_components/templatePagePreview.tsx, line 139 at r2 (raw file):

Previously, andrew-polk wrote…

I thought we specifically decided not to use the fault method here because RequiresBloomEnterprise already does the check.

Except that the fault method needs to control the pushToBottom div's existence, otherwise screen gets messed up.

[andrew-polk]

Reviewed 6 of 6 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gmartin7)

---

src/BloomBrowserUI/bookEdit/css/bloomDialog.less, line 2 at r2 (raw file):

Previously, gmartin7 (Gordon Martin) wrote…

I thought so, but it didn't work. The UI Font stack didn't get used when I used (reference).

I guess it needs the rest of the file to know where to find the fonts. So why does it work in editPaneGlobal.less?

[gmartin7]

Reviewable status: 15 of 29 files reviewed, 1 unresolved discussion (waiting on @andrew-polk)

---

src/BloomBrowserUI/bookEdit/css/bloomDialog.less, line 2 at r2 (raw file):

Previously, andrew-polk wrote…

I guess it needs the rest of the file to know where to find the fonts. So why does it work in editPaneGlobal.less?

I'm not sure it did! I think (reference) only works with importing constants. Besides now I've taken steps to use the bloomUI.less BODY font-family rule in the toolbox and in styleeditor's tabs and in qtips and lots of places!

[andrew-polk]

Reviewed 14 of 14 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gmartin7)

---

src/BloomBrowserUI/bookEdit/css/bloomDialog.less, line 2 at r2 (raw file):

Previously, gmartin7 (Gordon Martin) wrote…

I'm not sure it did! I think (reference) only works with importing constants. Besides now I've taken steps to use the bloomUI.less BODY font-family rule in the toolbox and in styleeditor's tabs and in qtips and lots of places!

I'm not excited about all those things changing as part of this change to the page selection dialog. What is the reason for doing it now?

---

src/BloomBrowserUI/bookEdit/css/editPaneGlobal.less, line 1 at r4 (raw file):

@import (reference) "../../bloomWebFonts.less";

Don't need this at all now, right? Probably other places?

[gmartin7]

Reviewable status: 25 of 29 files reviewed, 1 unresolved discussion (waiting on @andrew-polk)

---

src/BloomBrowserUI/bookEdit/css/bloomDialog.less, line 2 at r2 (raw file):

Previously, andrew-polk wrote…

I'm not excited about all those things changing as part of this change to the page selection dialog. What is the reason for doing it now?

It really simplifies where our font-family settings come from in general, and while maybe not really part of this card, JH had already added the rule to bloomUi.less that made it possible. And I'd rather take out a bunch of stuff that we don't need than add another complicated rule to make this one dialog work the way we want.

---

src/BloomBrowserUI/bookEdit/css/editPaneGlobal.less, line 1 at r4 (raw file):

Previously, andrew-polk wrote…

Don't need this at all now, right? Probably other places?

Done. And yes, I found at least one other place that no longer needed this, especially after deleting a bunch of unnecessary font-family rules.

[andrew-polk]

Approving but not merging as JT has unpublished comments.

Reviewed 4 of 4 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

---

src/BloomBrowserUI/bookEdit/css/bloomDialog.less, line 2 at r2 (raw file):

Previously, gmartin7 (Gordon Martin) wrote…

It really simplifies where our font-family settings come from in general, and while maybe not really part of this card, JH had already added the rule to bloomUi.less that made it possible. And I'd rather take out a bunch of stuff that we don't need than add another complicated rule to make this one dialog work the way we want.

I'm not going to ask for it this time because it isn't worth the effort/time.
But we always want to split out distinct changes whenever possible. In this case that might have meant making that change in a separate PR which got merged first so this could make use of it.
There are various reasons for this. One obvious one is now testers have to test the whole UI as part of this card.

[gmartin7]

Reviewable status: 28 of 29 files reviewed, all discussions resolved (waiting on @andrew-polk)

---

src/BloomBrowserUI/bookEdit/css/bloomDialog.less, line 2 at r2 (raw file):

Previously, andrew-polk wrote…

I'm not going to ask for it this time because it isn't worth the effort/time.
But we always want to split out distinct changes whenever possible. In this case that might have meant making that change in a separate PR which got merged first so this could make use of it.
There are various reasons for this. One obvious one is now testers have to test the whole UI as part of this card.

Yeah, you're right, of course. Or better would have been to get my PR working and in before master absorbed the MaterialUI changes that JH made. :(
I added a comment to the bloomUI.less file about why we can't use (reference) to import the font stack and what that means for using bloomUI.less too.

[JohnThomson]

Reviewable status: 28 of 29 files reviewed, 9 unresolved discussions (waiting on @andrew-polk and @gmartin7)

---

DistFiles/localization/en/Bloom.xlf, line 1612 at r4 (raw file):

        <note>ID: EditTab.RequiresEnterprise</note>
        <note>Appears in notice (possibly obscuring other controls when they may not be used) because no Bloom Enterprise project has been selected.</note>
      </trans-unit>

Discuss this with Steve. The wording is similar enough that translations of the old "EditTab.Toolbox.RequiresEnterprise" would probably still be better than nothing. Can we preserve them somehow?

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 100 at r4 (raw file):

            ".gridItemCaption",
            this._selectedGridItem
        ).text();

Are you sure they will be English (e.g., if that's not the current UI language)?

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 107 at r4 (raw file):

        const isEnterprise = $(this._selectedGridItem).hasClass(
            "enterprise-only-flag"
        );

We're supposed to be avoiding JQuery in new work. This is borderline, since some of the code above was pre-existing. But it would be a good chance to clean up. In any case, please keep in mind for future work.

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 558 at r4 (raw file):

                PageChooser.addPageClickHandler(
                    this._forChooseLayout,
                    this._selectedGridItem.attr("data-pageId"),

I don't understand why we're waiting to add a page click handler until some div gets double-clicked. Won't we add a new one each time it gets double-clicked, possibly with (confusingly) different values for the state of the check boxes? Of course a double-click usually closes the dialog, but it might not...at least some comments are needed.

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 565 at r4 (raw file):

                        ? convertAnywayCheckbox.checked
                        : false,
                    true, // If we're double-clicking on a page, let's assume we'll lose data until proven otherwise.

Why should we? How will it get proven otherwise?

---

src/BloomBrowserUI/react_components/templatePagePreview.tsx, line 1 at r4 (raw file):

import * as React from "react";

This control feels pretty specific to the Add Page dialog. I think it might better be added to the folder with that component's source than to one containing general-purpose components (at least until we find a second use for it).
I'd also like to find a better name...this control is doing a lot more than showing a preview.

---

src/BloomBrowserUI/react_components/templatePagePreview.tsx, line 55 at r4 (raw file):

        if (pageNeedsEnterprise) {
            thisPageNeedsEnterprise = pageNeedsEnterprise;
        }

It seems the above logic amounts to setting thisPageNeedsEnterprise to the same value as the pageNeedsEnterprise argument, so why introduce another variable and extra lines of code?

---

src/BloomBrowserUI/react_components/templatePagePreview.tsx, line 60 at r4 (raw file):

    const isAddPageButtonEnabled = (): boolean => {
        return !props.forChangeLayout || !props.willLoseData || continueChecked;

Don't we also require the absence of a bloom-enterprise-fault?

---

src/BloomBrowserUI/react_components/templatePagePreview.tsx, line 125 at r4 (raw file):

                        enabled={isAddPageButtonEnabled()}
                        onClick={() =>
                            addPageClickHandler(

Love to find a better name for this (and the ID) that somehow reflects that it might be either an Add Page or a Choose Layout button. How about useThisLayoutClickHandler? useThisPageClickHandler? addOrChoosePageClickHandler? (Note: names starting with 'use' may trigger lint warnings related to hooks.)

[andrew-polk]

Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @gmartin7 and @JohnThomson)

---

src/BloomBrowserUI/bookEdit/css/bloomDialog.less, line 2 at r2 (raw file):

Previously, gmartin7 (Gordon Martin) wrote…

Yeah, you're right, of course. Or better would have been to get my PR working and in before master absorbed the MaterialUI changes that JH made. :(
I added a comment to the bloomUI.less file about why we can't use (reference) to import the font stack and what that means for using bloomUI.less too.

Yes, I'm sure it has been annoying to get caught with two major refactors happening while this was being reviewed.

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 107 at r4 (raw file):

Previously, JohnThomson (John Thomson) wrote…

We're supposed to be avoiding JQuery in new work. This is borderline, since some of the code above was pre-existing. But it would be a good chance to clean up. In any case, please keep in mind for future work.

Thanks for pointing that out, JT. As you said, this is mostly existing. Let's not deal with it now.

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 558 at r4 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I don't understand why we're waiting to add a page click handler until some div gets double-clicked. Won't we add a new one each time it gets double-clicked, possibly with (confusingly) different values for the state of the check boxes? Of course a double-click usually closes the dialog, but it might not...at least some comments are needed.

The method name is very confusing. It isn't adding a click handler but rather handling the click on the add page button. This tripped me up a couple times, too, so it could be a good time to improve it. But I'm also ok just merging this languishing PR since the method was existing.

[hatton]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @gmartin7 and @JohnThomson)

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 107 at r4 (raw file):

Previously, andrew-polk wrote…

Thanks for pointing that out, JT. As you said, this is mostly existing. Let's not deal with it now.

Sigh there's always an appetite to grab the next card, never to to improve as we go.

[gmartin7]

Reviewable status: 25 of 30 files reviewed, 9 unresolved discussions (waiting on @andrew-polk and @JohnThomson)

---

DistFiles/localization/en/Bloom.xlf, line 1612 at r4 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Discuss this with Steve. The wording is similar enough that translations of the old "EditTab.Toolbox.RequiresEnterprise" would probably still be better than nothing. Can we preserve them somehow?

Actually, the text won't be this anyway. I'll modify it now, so that we know it's not that simple. The eventual UI will use two different strings (this one and EditTab.EnterpriseSettingsButton).

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 100 at r4 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Are you sure they will be English (e.g., if that's not the current UI language)?

Yep. These are the page labels direct from Bloom's template files (e.g. browser/templates/template books/Basic Book.html). The only exception might be a "home grown" template, but in that case we can't expect to have a localization on file anyway.

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 107 at r4 (raw file):

Previously, JohnThomson (John Thomson) wrote…

We're supposed to be avoiding JQuery in new work. This is borderline, since some of the code above was pre-existing. But it would be a good chance to clean up. In any case, please keep in mind for future work.

I think it was all pre-existing. I could go through and rework this part, but it seems like this PR has already grown past its mandate...

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 558 at r4 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I don't understand why we're waiting to add a page click handler until some div gets double-clicked. Won't we add a new one each time it gets double-clicked, possibly with (confusingly) different values for the state of the check boxes? Of course a double-click usually closes the dialog, but it might not...at least some comments are needed.

I believe (if I understand the code correctly) that what we're doing is binding a function as the double-click handler to each .invisibleThumbCover in the currentGroup of pages. Then when the page gets double-clicked, it goes off and runs that function.
Anyway, it seems to work fine.

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 565 at r4 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Why should we? How will it get proven otherwise?

OK, I was lazy here. I wasn't sure it would work to do what needed to be done. I was wrong; it works fine!

---

src/BloomBrowserUI/react_components/templatePagePreview.tsx, line 1 at r4 (raw file):

Previously, JohnThomson (John Thomson) wrote…

This control feels pretty specific to the Add Page dialog. I think it might better be added to the folder with that component's source than to one containing general-purpose components (at least until we find a second use for it).
I'd also like to find a better name...this control is doing a lot more than showing a preview.

The location idea was vaguely in the back of my mind already. Done.

---

src/BloomBrowserUI/react_components/templatePagePreview.tsx, line 55 at r4 (raw file):

Previously, JohnThomson (John Thomson) wrote…

It seems the above logic amounts to setting thisPageNeedsEnterprise to the same value as the pageNeedsEnterprise argument, so why introduce another variable and extra lines of code?

!!true 😁 (pageNeedsEnterprise type isboolean | undefined, but there's a better way to handle that.

---

src/BloomBrowserUI/react_components/templatePagePreview.tsx, line 60 at r4 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Don't we also require the absence of a bloom-enterprise-fault?

In the absence of said bloom-enterprise-fault, the button in question won't even be on the page (because of conditional rendering involving !enterpriseSubscriptionFault).

---

src/BloomBrowserUI/react_components/templatePagePreview.tsx, line 125 at r4 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Love to find a better name for this (and the ID) that somehow reflects that it might be either an Add Page or a Choose Layout button. How about useThisLayoutClickHandler? useThisPageClickHandler? addOrChoosePageClickHandler? (Note: names starting with 'use' may trigger lint warnings related to hooks.)

Changed the handler name to your suggestion that doesn't have the use prefix. Changed the ID to addOrChoosePageButton.

[StephenMcConnel]

Reviewable status: 25 of 30 files reviewed, 9 unresolved discussions (waiting on @andrew-polk and @JohnThomson)

---

DistFiles/localization/en/Bloom.xlf, line 1612 at r4 (raw file):

Previously, gmartin7 (Gordon Martin) wrote…

Actually, the text won't be this anyway. I'll modify it now, so that we know it's not that simple. The eventual UI will use two different strings (this one and EditTab.EnterpriseSettingsButton).

Possible steps we could take when an English string changes in ways that we consider minimal.

1. Do nothing.
   PRO - a) no extra work for programmers, b) no possible confusion for "who approved this nonsense"
   CON - a) no translation available in Bloom for some period of time when there used to be a translation, b) unhappy translators that we keep causing them work

2. Manually go through all the translations in all the languages on Crowdin and reapprove the old translations. (possibly copying them to a new id?)
   PRO - a) preserve Bloom having a probably adequate translation
   CON - a) programmers touching language data they don't understand????!!, b) translators likely never noticing a new translation is needed, c) translators wondering who approved the obviously wrong translation when it gets noticed (potential political issue)

3. Preserve existing translations in the xliff files in the Bloom repository until new translations are made and approved on Crowdin. (and then merged into the Bloom repo)
   PRO - a) translators notice need for translation/approval immediately, b) programmers don't have to touch language data on Crowdin
   CON - a) unknown amount of work added when merging translations from Crowdin into Bloom to ignore updates we don't want. (If this grows beyond one or two strings, I'm not sure I even want to think about it!!)

I've used approach #2 once when the only changes were removing leading and trailing whitespace. I wouldn't want to use it in any other circumstance. Approach #1 is really the only feasible one in the general case as far as I can tell.

Note that if the id is different, even if the English strings are identical, Crowdin won't automatically preserve approval status even if it copies the translation suggestion from the older id.

[andrew-polk]

---

DistFiles/localization/en/Bloom.xlf, line 1612 at r4 (raw file):

Previously, StephenMcConnel (Steve McConnel) wrote…

Possible steps we could take when an English string changes in ways that we consider minimal.

1. Do nothing.
   PRO - a) no extra work for programmers, b) no possible confusion for "who approved this nonsense"
   CON - a) no translation available in Bloom for some period of time when there used to be a translation, b) unhappy translators that we keep causing them work

2. Manually go through all the translations in all the languages on Crowdin and reapprove the old translations. (possibly copying them to a new id?)
   PRO - a) preserve Bloom having a probably adequate translation
   CON - a) programmers touching language data they don't understand????!!, b) translators likely never noticing a new translation is needed, c) translators wondering who approved the obviously wrong translation when it gets noticed (potential political issue)

3. Preserve existing translations in the xliff files in the Bloom repository until new translations are made and approved on Crowdin. (and then merged into the Bloom repo)
   PRO - a) translators notice need for translation/approval immediately, b) programmers don't have to touch language data on Crowdin
   CON - a) unknown amount of work added when merging translations from Crowdin into Bloom to ignore updates we don't want. (If this grows beyond one or two strings, I'm not sure I even want to think about it!!)

I've used approach #2 once when the only changes were removing leading and trailing whitespace. I wouldn't want to use it in any other circumstance. Approach #1 is really the only feasible one in the general case as far as I can tell.

Note that if the id is different, even if the English strings are identical, Crowdin won't automatically preserve approval status even if it copies the translation suggestion from the older id.

Another CON for #1:
We risk pushing the resulting "disapproval" of the translation back to a version which doesn't even have the new text.
Result: Release and/or beta had a good translation. Now they see English. This might get fixed if translators retranslate and we repush. But if it is a new ID, it will never get fixed.

Another thing we have done in the past is used a new ID, left the old one for 2 or 3 versions so no version loses something which was already translated. Steve has typically put notes on the new and old ones with the version numbers of each.

[andrew-polk]

Reviewed 4 of 5 files at r7.
Reviewable status: 29 of 30 files reviewed, 9 unresolved discussions (waiting on @JohnThomson)

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 107 at r4 (raw file):

Previously, gmartin7 (Gordon Martin) wrote…

I think it was all pre-existing. I could go through and rework this part, but it seems like this PR has already grown past its mandate...

In this case, I agree with Gordon. There is other work held up currently by this change.

---

src/BloomBrowserUI/react_components/templatePagePreview.tsx, line 1 at r4 (raw file):

Previously, gmartin7 (Gordon Martin) wrote…

The location idea was vaguely in the back of my mind already. Done.

Looks like you added the new one without deleting the old one.

[gmartin7]

Reviewable status: 29 of 30 files reviewed, 9 unresolved discussions (waiting on @JohnThomson)

---

src/BloomBrowserUI/react_components/templatePagePreview.tsx, line 1 at r4 (raw file):

Previously, andrew-polk wrote…

Looks like you added the new one without deleting the old one.

That would explain why Reviewable didn't do what I expected...

[JohnThomson]

Reviewable status: 27 of 29 files reviewed, all discussions resolved (waiting on @andrew-polk)

---

DistFiles/localization/en/Bloom.xlf, line 1612 at r4 (raw file):

Previously, andrew-polk wrote…

Another CON for #1:
We risk pushing the resulting "disapproval" of the translation back to a version which doesn't even have the new text.
Result: Release and/or beta had a good translation. Now they see English. This might get fixed if translators retranslate and we repush. But if it is a new ID, it will never get fixed.

Another thing we have done in the past is used a new ID, left the old one for 2 or 3 versions so no version loses something which was already translated. Steve has typically put notes on the new and old ones with the version numbers of each.

Sounds like in this case we won't end up being able to use the old translation. Resolving.

---

src/BloomBrowserUI/pageChooser/page-chooser.ts, line 558 at r4 (raw file):

PageChooser.addPageClickHandler

Ugh. So PageChooser.addPageClickHandler doesn't add a click handler to anything...rather it IS the click handler for the AddPage button. Maybe we should have a card to do follow-up cleanup on this PR, since we're eager to get it in? Anyway, it seems there's not a problem here except naming.

[JohnThomson]

Reviewed 6 of 15 files at r1, 2 of 12 files at r2, 1 of 6 files at r3, 10 of 14 files at r4, 3 of 4 files at r5, 1 of 1 files at r6, 5 of 5 files at r7, 2 of 2 files at r8.
Reviewable status: complete! all files reviewed, all discussions resolved