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

fix(ui-tablemode): adjust the size of the column with the initial data #2320

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

skamril
Copy link
Member

@skamril skamril commented Jan 28, 2025

ANT-1217

@skamril skamril added bug Something isn't working front-end labels Jan 28, 2025
@skamril skamril requested a review from hdinia January 28, 2025 12:45
@skamril skamril self-assigned this Jan 28, 2025
@skamril skamril force-pushed the bugfix/1217-tablemode-columns-size branch 5 times, most recently from 4c18a03 to 0796e7f Compare January 28, 2025 13:55
@skamril skamril changed the title fix(ui-tablemode): adapte the size of the column with the data fix(ui-tablemode): adapt the size of the column with the data Jan 28, 2025
@skamril skamril changed the title fix(ui-tablemode): adapt the size of the column with the data fix(ui-tablemode): adjust the size of the column with the initial data Jan 28, 2025
Comment on lines 15 to 65
/**
* Gets the computed style of the given element.
*
* @see https://stackoverflow.com/a/21015393
*
* @param element - The element to get the style from.
* @param prop - The property to get the value of.
* @returns The computed style of the given element.
*/
export function getCssStyle(element: HTMLElement, prop: string) {
return window.getComputedStyle(element, null).getPropertyValue(prop);
}

/**
* Gets the font of the given element, or the `body` element if none is provided.
* The returned value follows the CSS `font` shorthand property format.
*
* @see https://stackoverflow.com/a/21015393
*
* @param element - The element to get the font from.
* @returns The font of the given element.
*/
export function getCssFont(element = document.body) {
const fontWeight = getCssStyle(element, "font-weight") || "normal";
const fontSize = getCssStyle(element, "font-size") || "16px";
const fontFamily = getCssStyle(element, "font-family") || "Arial";

return `${fontWeight} ${fontSize} ${fontFamily}`;
}

/**
* Uses `canvas.measureText` to compute and return the width of the specified text,
* using the specified canvas font if defined (use font from the "body" otherwise).
*
* @see https://stackoverflow.com/a/21015393
*
* @param text - The text to be rendered.
* @param [font] - The CSS font that text is to be rendered with (e.g. "bold 14px Arial").
* @returns The width of the text in pixels.
*/
export function measureTextWidth(text: string, font?: string) {
const canvas = document.createElement("canvas");
const context = canvas.getContext("2d");
if (context) {
context.font = font || getCssFont();
return context.measureText(text).width;
}
return 0;
}
Copy link
Member

@hdinia hdinia Jan 28, 2025

Choose a reason for hiding this comment

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

update measureTextWidth to use a factory function instead:

/**
 * Uses `canvas.measureText` to compute and return the width of the specified text,
 * using the specified canvas font if defined (use font from the "body" otherwise).
 *
 * @see https://stackoverflow.com/a/21015393
 *
 * @returns The width of the text in pixels.
 */
export function createTextMeasurer() {
  const canvas = document.createElement("canvas");
  const context = canvas.getContext("2d");
  const font = getFont();

  if (!context) {
    return (text: string) => 0;
  }

  context.font = font;

  return (text: string) => context.measureText(text).width;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

You forget font argument, otherwise I create the canvas element outside, it's enough

Comment on lines +113 to +128
const columnsWithAdjustedSize = useMemo(
() =>
columns.map((column) => ({
...column,
width:
"width" in column
? column.width
: Math.max(
measureTextWidth(column.title),
...rowNames.map((rowName) =>
measureTextWidth(defaultData[rowName][column.id].toString()),
),
),
})),
[columns, defaultData, rowNames],
);
Copy link
Member

@hdinia hdinia Jan 28, 2025

Choose a reason for hiding this comment

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

here a canvas is re-created at each iteration in the Math.max(), it's expensive

  const columnsWithAdjustedSize = useMemo(() => {
    const measureText = createTextMeasurer();

    return columns.map((column) => {
      if ("width" in column) {
        return column;
      }

      const maxWidth = Math.max(
        measureText(column.title),
        ...rowNames.map((rowName) => measureText(defaultData[rowName][column.id].toString())),
      );

      return { ...column, width: maxWidth };
    });
  }, [columns, defaultData, rowNames]);

see comment about domUtils.ts file for the createTextMeasurer() util using the factory function pattern to avoid re-creation of canvas

Comment on lines +132 to +140
const rowMarkers = useMemo<RowMarkers>(
() =>
rowMarkersFromProps || {
kind: "clickable-string",
getTitle: (index) => rowNames[index],
width: Math.max(...rowNames.map((name) => measureTextWidth(name))),
},
[rowMarkersFromProps, rowNames],
);
Copy link
Member

Choose a reason for hiding this comment

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

const rowMarkers = useMemo<RowMarkers>(() => {
    if (rowMarkersFromProps) {
      return rowMarkersFromProps;
    }

    const measureText = createTextMeasurer();
    const maxWidth = Math.max(...rowNames.map(measureText));

    return {
      kind: "clickable-string",
      getTitle: (index) => rowNames[index],
      width: maxWidth,
    };
  }, [rowMarkersFromProps, rowNames]);

Comment on lines 81 to 87
useEffect(() => {
setColumns(
isStringRowMarkers ? [{ id: "", title: "" }, ...columnsFromProps] : columnsFromProps,
isStringRowMarkers
? [{ id: "", title: "", width: rowMarkersOptions.width }, ...columnsFromProps]
: columnsFromProps,
);
}, [columnsFromProps, isStringRowMarkers]);
}, [columnsFromProps, isStringRowMarkers, rowMarkersOptions.width]);
Copy link
Member

Choose a reason for hiding this comment

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

here about the useEffect, not sure about it but seems not necessary no ?

  const columns = useMemo(() => {
    if (isStringRowMarkers) {
      return [{ id: "", title: "", width: rowMarkersOptions.width }, ...columnsFromProps];
    }

    return columnsFromProps;
  }, [columnsFromProps, isStringRowMarkers, rowMarkersOptions.width]);

Copy link
Member Author

Choose a reason for hiding this comment

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

setColumns is used for columns resizing

@skamril skamril force-pushed the bugfix/1217-tablemode-columns-size branch 3 times, most recently from 2dd5c14 to 997c0ff Compare January 28, 2025 15:34
@skamril skamril force-pushed the bugfix/1217-tablemode-columns-size branch from 997c0ff to 8b92e53 Compare January 28, 2025 15:36
@skamril skamril merged commit 07a0c1f into dev Jan 28, 2025
14 checks passed
@skamril skamril deleted the bugfix/1217-tablemode-columns-size branch January 28, 2025 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working front-end size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants