refactor(ui): refactor capitalization of platform name and sub types #7099

Masterchen09 · 2023-01-22T18:04:56Z

I was really surprised and actually can't believe that no one else has noticed it yet and asked about it on Slack, but the capitalization of the platform names (and the sub types) is currently inconsistent: When you navigate through the entities, lineage, etc. sometimes the platform names are correctly capitalized and sometimes not. Therefore, I have took the time and checked all occurrences of capitalizeFirstLetter and capitalizeFirstLetterOnly in relation to the platform name and the sub types:

If a display name is available for a platform, the display name should be visible.
The capitalization of the display name of a platform should never be changed, because we should assume that the display name is already correct.
If no display name is available the name of the platform should be visible.
The name of the platform seems to have been capitalized in the past, therefore it should be capitalized everywhere where it is used when there is no display name available.
- Even though we could discuss whether this makes sense or not: Assuming that we do not have a display name for the MySQL platform, Mysql after capitalization instead of mysql is not really better, as the correct display name would be MySQL? Therefore, we could also think about to completely remove the capitalization for the platform names?
For the sub types it is a bit easier, as there is currently no display name, therefore everywhere where the sub types are displayed, they should be capitalized.
- This is also something we could discuss: Does it makes sense to change the capitalization of the sub types, I have found places where the sub types are ingested with the correct casing (see here), but there are also places where the sub types are ingested with lowercase letters only (see here). I am not sure whether there are some best practices how sub types should be ingested (or if there are any ideas/plans for enhancing this in general, e.g. by introducing a new sub type entity with a display name where the sub types aspect is then only referencing to the sub types entity), but I think as long lowercase-only sub types are valid, the sub types should be capitalized in the UI.
In all cases capitalizeFirstLetterOnly and not capitalizeFirstLetter should be used to support sub types consisting of multiple words, where the casing is already correct (e.g., "My Super Dashboard" instead of "My super dashboard" after using capitalizeFirstLetter):
- Alternatively, we could also introduce a new capitalization function which only changes the capitalization when all letters of the sub type are in lowercase...but capitalizeFirstLetterOnly is also sufficient for most of the cases (capitalizeFirstLetterOnly does only not work for cases where the first character should be in lowercase intentionally).

P.S.: The removal of the meta-favicon.ico has nothing to do with the capitalization, but I think it was added by mistake, because it is not used anywhere, therefore I have removed it.

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

jjoyce0510 · 2023-01-24T02:20:29Z

datahub-web-react/src/app/entity/chart/ChartEntity.tsx

@@ -197,9 +198,9 @@ export class ChartEntity implements Entity<Chart> {
    getLineageVizConfig = (entity: Chart) => {
        return {
            urn: entity.urn,
-            name: entity.properties?.name || '',
+            name: entity.properties?.name || entity.urn,


jjoyce0510 · 2023-01-24T02:21:18Z

datahub-web-react/src/app/entity/chart/preview/ChartPreview.tsx

@@ -62,7 +61,6 @@ export const ChartPreview = ({
    snippet?: React.ReactNode | null;
 }): JSX.Element => {
    const entityRegistry = useEntityRegistry();
-    const capitalizedPlatform = capitalizeFirstLetter(platform);


jjoyce0510 · 2023-01-24T02:22:19Z

datahub-web-react/src/app/entity/container/preview/Preview.tsx

@@ -62,7 +63,7 @@ export const Preview = ({
    externalUrl?: string | null;
 }): JSX.Element => {
    const entityRegistry = useEntityRegistry();
-    const typeName = (subTypes?.typeNames?.length && subTypes?.typeNames[0]) || 'Container';
+    const typeName = capitalizeFirstLetterOnly(subTypes?.typeNames?.[0]) || 'Container';


Minor: Will this work when

subTypes?.typeNames is an empty array?. If not, seems a bit worrisome to remove the first check.

datahub-web-react/src/app/entity/mlFeature/MLFeatureEntity.tsx

jjoyce0510 · 2023-01-24T02:35:14Z

datahub-web-react/src/app/entity/shared/containers/profile/header/EntityHeader.tsx

    const externalUrl = entityData?.externalUrl || undefined;
    const entityCount = entityData?.entityCount;
    const isCompact = React.useContext(CompactContext);

    const entityName = entityData?.name;
-    const subType =
-        (entityData?.subTypes?.typeNames?.length || 0) > 0 ? entityData?.subTypes?.typeNames![0] : undefined;
+    const subType = capitalizeFirstLetterOnly(entityData?.subTypes?.typeNames?.[0]) || undefined;


Is it safe to remove this check? entityData?.subTypes?.typeNames?.length

jjoyce0510 · 2023-01-24T02:36:36Z

...src/app/entity/shared/containers/profile/header/PlatformContent/PlatformContentContainer.tsx

-    return entityData?.entityTypeOverride || entityTypeCased || '';
+    return (
+        entityData?.entityTypeOverride ||
+        capitalizeFirstLetterOnly(entityData?.subTypes?.typeNames?.[0]) ||


is it safe to remove the check entityData?.subTypes?.typeNames?.length

What if type names does not exist?

(I see that the method can deal with undefined), but I think the other case is when the array exists, but is empty (technically a possible state)

jjoyce0510 · 2023-01-24T02:39:56Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Validations/DatasetAssertionsList.tsx

+                            )) || (
+                                <Typography.Text>
+                                    {record.platform.properties?.displayName ||
+                                        capitalizeFirstLetterOnly(record.platform.name)}


Thank you for adding this!

jjoyce0510 · 2023-01-24T02:40:12Z

datahub-web-react/src/app/entity/shared/utils.ts

@@ -71,7 +72,7 @@ export const singularizeCollectionName = (collectionName: string): string => {
 };

 export function getPlatformName(entityData: GenericEntityProperties | null) {
-    return entityData?.platform?.properties?.displayName || entityData?.platform?.name;
+    return entityData?.platform?.properties?.displayName || capitalizeFirstLetterOnly(entityData?.platform?.name);


Thank you!!

jjoyce0510 · 2023-01-24T02:40:33Z

datahub-web-react/src/app/ingest/source/IngestionSourceList.tsx

@@ -266,7 +266,7 @@ export const IngestionSourceList = () => {
                message.success({ content: 'Removed ingestion source.', duration: 2 });
                const newRemovedUrns = [...removedUrns, urn];
                setRemovedUrns(newRemovedUrns);
-                setTimeout(function () {
+                setTimeout(() => {


Not sure why these changes are strictly necessary, but we will leave them

Oh, I forgot to mention it: ESLint reported it as an unexpected unnamed function...everywhere else in the code an arrow function was used, so I changed it.

jjoyce0510 · 2023-01-24T02:41:07Z

datahub-web-react/src/app/lineage/LineageEntityNode.tsx

    if (node.data.siblingPlatforms && !isHideSiblingMode) {
        platformDisplayText = node.data.siblingPlatforms
-            .map((platform) => platform.properties?.displayName || platform.name)
+            .map((platform) => platform.properties?.displayName || capitalizeFirstLetterOnly(platform.name))


jjoyce0510 · 2023-01-24T02:41:38Z

datahub-web-react/src/app/lineage/__tests__/LineageEntityView.test.tsx

@@ -32,7 +32,7 @@ describe('LineageEntityView', () => {
    it('should not render a divider if there is no platform name', () => {
        const datasetNoPlatformName = {
            ...dataset1,
-            platform: { ...dataset1.platform, properties: { displayName: '' } },
+            platform: { ...dataset1.platform, name: '', properties: { displayName: '' } },


Why empty string ?

Before the changes in LinageEntityView.tsx only the displayName of the platform was used, therefore the name of the platform must also be set to an empty string (or undefined) for this test to be successful. If I think about it again, in theory we could also remove this test completely, because the platform name should never be empty, however as the platform in GenericEntityProperties is optional it is possible that getPlatformName returns undefined...in this case the test has somehow the right to exist...

jjoyce0510 · 2023-01-24T02:42:23Z

datahub-web-react/src/app/recommendations/renderer/component/EntityNameList.tsx

-                const platformName =
-                    genericProps?.platform?.properties?.displayName ||
-                    capitalizeFirstLetter(genericProps?.platform?.name);
+                const platformName = getPlatformName(genericProps);


jjoyce0510

So firstly, I in large part agree with your assessment.

Platform Name should only be capitalized (first letter) if there is not display name set.
Sub Type name should only be capitalized for the first letter (in all cases)

It seems that this PR covers it!

I think there's just one case that was sometimes previously covered but now has been removed in some places: checking for the presence of the subTypes array itself.

Other than this, I think this PR is largely ready go. I am very grateful you took the time to do this!

Masterchen09 · 2023-01-24T19:34:39Z

The length-check for the typeNames array is not neccessary, because it is fine to access an index of an array which does not exist (in JavaScript/TypeScript), the resulting value is then undefined and the right-hand value - the entity type - of the OR will be used. The case that typeNames array does not exist at all is covered using the optional chaining (then the resulting value is also undefined) and besides that I think it is safe to assume that typeNames is an array if it is not undefined, because that's how it is defined in the schema.

jjoyce0510

LGTM - Thank you for adding!

jjoyce0510 · 2023-01-25T04:20:32Z

One more thing! Looks like this has caused a smoke test to fail.. Can you change siblings.js to lower case "Dbt" to "dbt" here: https://github.com/datahub-project/datahub/blob/master/smoke-test/tests/cypress/cypress/integration/siblings/siblings.js#L127

jjoyce0510 · 2023-01-25T04:20:50Z

Once we have that small change, we are good to merge.

Thanks
John

…atahub-project#7099)

github-actions bot added the product PR or Issue related to the DataHub UI/UX label Jan 22, 2023

Masterchen09 force-pushed the refactor-capitalize-platform-name branch from a039529 to f1092ef Compare January 22, 2023 20:07

jjoyce0510 reviewed Jan 24, 2023

View reviewed changes

datahub-web-react/src/app/entity/mlFeature/MLFeatureEntity.tsx Show resolved Hide resolved

jjoyce0510 reviewed Jan 24, 2023

View reviewed changes

anshbansal added the community-contribution PR or Issue raised by member(s) of DataHub Community label Jan 24, 2023

jjoyce0510 approved these changes Jan 25, 2023

View reviewed changes

Masterchen09 force-pushed the refactor-capitalize-platform-name branch from a10aa7e to fd9c0bf Compare January 25, 2023 13:57

refactor(ui): refactor capitalization of platform name and sub types

e573d22

Masterchen09 force-pushed the refactor-capitalize-platform-name branch from fd9c0bf to e573d22 Compare January 25, 2023 15:10

jjoyce0510 merged commit 566b7b0 into datahub-project:master Jan 25, 2023

Masterchen09 deleted the refactor-capitalize-platform-name branch January 25, 2023 18:25

ericyomi pushed a commit to ericyomi/datahub that referenced this pull request Feb 8, 2023

refactor(ui): refactor capitalization of platform name and sub types (d…

bb2f5a0

…atahub-project#7099)

oleg-ruban pushed a commit to RChygir/datahub that referenced this pull request Feb 28, 2023

refactor(ui): refactor capitalization of platform name and sub types (d…

76a825d

…atahub-project#7099)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ui): refactor capitalization of platform name and sub types #7099

refactor(ui): refactor capitalization of platform name and sub types #7099

Masterchen09 commented Jan 22, 2023

jjoyce0510 Jan 24, 2023

jjoyce0510 Jan 24, 2023

jjoyce0510 Jan 24, 2023

jjoyce0510 Jan 24, 2023

jjoyce0510 Jan 24, 2023

jjoyce0510 Jan 24, 2023

jjoyce0510 Jan 24, 2023

jjoyce0510 Jan 24, 2023

jjoyce0510 Jan 24, 2023

Masterchen09 Jan 24, 2023

jjoyce0510 Jan 24, 2023

jjoyce0510 Jan 24, 2023

Masterchen09 Jan 24, 2023

jjoyce0510 Jan 24, 2023

jjoyce0510 left a comment

Masterchen09 commented Jan 24, 2023

jjoyce0510 left a comment

jjoyce0510 commented Jan 25, 2023

jjoyce0510 commented Jan 25, 2023

refactor(ui): refactor capitalization of platform name and sub types #7099

refactor(ui): refactor capitalization of platform name and sub types #7099

Conversation

Masterchen09 commented Jan 22, 2023

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjoyce0510 left a comment

Choose a reason for hiding this comment

Masterchen09 commented Jan 24, 2023

jjoyce0510 left a comment

Choose a reason for hiding this comment

jjoyce0510 commented Jan 25, 2023

jjoyce0510 commented Jan 25, 2023