tbonelee commented on code in PR #5111:
URL: https://github.com/apache/zeppelin/pull/5111#discussion_r2509795414
##########
zeppelin-web-angular/e2e/tests/notebook/published/published-paragraph.spec.ts:
##########
@@ -99,6 +99,23 @@ test.describe('Published Paragraph', () => {
timeout: 10000
});
});
+
+ test('should enter published paragraph in React mode via URL with
react=true', async ({ page }) => {
+ await test.step('Given I navigate to React mode URL', async () => {
+ const reactModeUrl =
`/#/notebook/${testNotebook.noteId}/paragraph/${testNotebook.paragraphId}?react=true`;
+ await page.goto(reactModeUrl);
+ await waitForZeppelinReady(page);
+
+ await
page.waitForURL(`**/${testNotebook.noteId}/paragraph/${testNotebook.paragraphId}*`,
{
+ timeout: 15000
+ });
+ });
+
+ await test.step('Then React mode should be active', async () => {
+ const currentUrl = page.url();
+ expect(currentUrl).toContain('react=true');
+ });
+ });
Review Comment:
If there's no special handling in place, navigating to a URL that includes
query parameters will always result in those parameters being present in the
final URL. In that case, wouldn't this test always pass regardless of whether
React mode is actually active?
It may be better to either remove this test or revise it to validate React
mode using a different, more reliable indicator.
##########
zeppelin-web-angular/projects/zeppelin-react/src/components/renderers/TextRenderer.tsx:
##########
Review Comment:
We should handle ANSI color escape codes. See
https://github.com/apache/zeppelin/blob/bf62a2a25f460aaaa4794f82a0b52553187eeb7f/zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.ts#L367
##########
zeppelin-web-angular/projects/zeppelin-react/src/components/common/Empty.tsx:
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { Alert } from 'antd';
+
+export const Empty = () => {
+ return (
+ <Alert
+ message="No Data"
+ description="No paragraph data found"
+ type="warning"
+ showIcon
+ />
+ );
+};
Review Comment:
A new line at the EOF is needed.
##########
zeppelin-web-angular/projects/zeppelin-react/src/components/common/Loading.tsx:
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { Spin, Typography } from 'antd';
+
+export const Loading = () => {
+ return (
+ <div style={{ textAlign: 'center' }}>
+ <Spin size="large" />
+ <Typography.Title level={4} style={{ marginTop: 16 }}>
+ Loading paragraph data...
+ </Typography.Title>
+ </div>
+ );
+};
Review Comment:
A new line at the EOF is needed.
##########
zeppelin-web-angular/e2e/tests/notebook/published/published-paragraph.spec.ts:
##########
@@ -138,4 +155,53 @@ test.describe('Published Paragraph', () => {
await runButton.click();
await expect(modal).toBeHidden();
});
+
+ test('should show confirmation modal in React mode and allow running the
paragraph', async ({ page }) => {
+ const { noteId, paragraphId } = testNotebook;
+
+ await test.step('Given I clear paragraph output in normal notebook view',
async () => {
+ await publishedParagraphPage.navigateToNotebook(noteId);
+
+ const paragraphElement =
page.locator('zeppelin-notebook-paragraph').first();
+ const paragraphResult =
paragraphElement.locator('zeppelin-notebook-paragraph-result');
+
+ // Only clear output if result exists
+ if (await paragraphResult.isVisible()) {
+ const settingsButton = paragraphElement.locator('a[nz-dropdown]');
+ await settingsButton.click();
+
+ const clearOutputButton = page.locator('li.list-item:has-text("Clear
output")');
+ await clearOutputButton.click();
+ await expect(paragraphResult).toBeHidden();
+ }
Review Comment:
I think test scenarios should be as deterministic as possible.
Since `beforeEach` creates a new notebook, we may not need a conditional
`if` block here to clear the output.
It should be sufficient to assert that the paragraph result is hidden with
something like `expect(paragraphResult).toBeHidden()`.
##########
zeppelin-web-angular/projects/zeppelin-react/src/components/renderers/HTMLRenderer.tsx:
##########
Review Comment:
Maybe we should highlight code blocks like the Angular version.
Please check
https://github.com/apache/zeppelin/blob/bf62a2a25f460aaaa4794f82a0b52553187eeb7f/zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.ts#L319.
##########
zeppelin-web-angular/projects/zeppelin-react/src/hooks/useTableExport.ts:
##########
Review Comment:
It seems that, at this point, this doesn’t necessarily need to be a React
custom hook and could simply be implemented as a regular utility function.
Do you have any plans for it to hold state or anything similar?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
