Skip to content

Conversation

@kc-leung
Copy link
Contributor

No description provided.

@github-actions
Copy link

github-actions bot commented May 12, 2022

📚 Storybook preview (updated to 34d9d8f)

@amy83762100
Copy link
Contributor

The width of the input field seems to need to be changed. The last letter in the input field disappeared on my side.
image

@geoerika
Copy link
Contributor

There is also a clear icon that appears in editor mode that doesn't seem to do anything.
Screen Shot 2022-05-16 at 8 48 16 PM

Copy link

@sallkall sallkall left a comment

Choose a reason for hiding this comment

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

@kc-leung please double check the styling for each state (you can use the current QL implementation for reference @hyx131 would know the details). figma reference
image

@kc-leung
Copy link
Contributor Author

@kc-leung please double check the styling for each state (you can use the current QL implementation for reference @hyx131 would know the details). figma reference image

could you provide me the figma link??

@sallkall
Copy link

@kc-leung please double check the styling for each state (you can use the current QL implementation for reference @hyx131 would know the details). figma reference image

could you provide me the figma link??

https://www.figma.com/file/3UGpxxcciW9bhWqC4ZjL7N/Query-Lab?node-id=2667%3A204232

@kc-leung
Copy link
Contributor Author

@sallkall do you want the title to have the delete icon at the end ??

@geoerika
Copy link
Contributor

geoerika commented Jul 5, 2022

@kc-leung could you please sort out & finish this branch as well?

@kc-leung
Copy link
Contributor Author

kc-leung commented Jul 5, 2022

@kc-leung could you please sort out & finish this branch as well?

I will ask veronika to take a look

@kc-leung kc-leung requested review from sallkall and vlatawiec July 6, 2022 13:45
Copy link

@vlatawiec vlatawiec left a comment

Choose a reason for hiding this comment

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

This solves the issue of title text getting cut off, but in extreme circumstances, longer titles now push/condense the right-most buttons and tags (as shown below). This will need to be fixed at some point with some sort of upper limit on character input.
image
Otherwise looks great! :)

@kc-leung
Copy link
Contributor Author

kc-leung commented Jul 6, 2022

This solves the issue of title text getting cut off, but in extreme circumstances, longer titles now push/condense the right-most buttons and tags (as shown below). This will need to be fixed at some point with some sort of upper limit on character input. image Otherwise looks great! :)

thanks for the review! the width is automatically calculated based on the length of title since some words/special characters are smaller, it causes that issue. Any suggestion on how we can get the width of input box grow automatically? @geoerika @hyx131 @amy83762100

@kc-leung kc-leung requested a review from geoerika July 6, 2022 14:36
alignItems: 'center',
margin: '0 0.6rem',
display: 'flex',

Copy link
Contributor

Choose a reason for hiding this comment

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

no need for empty line here, please.

@geoerika
Copy link
Contributor

geoerika commented Jul 7, 2022

This solves the issue of title text getting cut off, but in extreme circumstances, longer titles now push/condense the right-most buttons and tags (as shown below). This will need to be fixed at some point with some sort of upper limit on character input. image Otherwise looks great! :)

thanks for the review! the width is automatically calculated based on the length of title since some words/special characters are smaller, it causes that issue. Any suggestion on how we can get the width of input box grow automatically? @geoerika @hyx131 @amy83762100

Of the top of my head, I can only think of measuring the rendered text each time it changes, but it might be overkill. I used this method to calculate stuff in chart-system: https://stackoverflow.com/questions/118241/calculate-text-width-with-javascript/21015393#21015393

@geoerika
Copy link
Contributor

geoerika commented Jul 7, 2022

@kc-leung don't forget to implement the delete function of the delete icon in the text field... or it's just not working? Should we remove it, because right now it doesn't do anything.

@kc-leung
Copy link
Contributor Author

kc-leung commented Jul 7, 2022

@kc-leung don't forget to implement the delete function of the delete icon in the text field... or it's just not working? Should we remove it, because right now it doesn't do anything.

doesn't it work for you? Let rebase the branch and get the latest version of Lumen-Labs

@kc-leung kc-leung requested review from geoerika and removed request for sallkall August 15, 2022 14:18
@geoerika
Copy link
Contributor

geoerika commented Aug 22, 2022

Of the top of my head, I can only think of measuring the rendered text each time it changes, but it might be overkill. I used this method to calculate stuff in chart-system: https://stackoverflow.com/questions/118241/calculate-text-width-with-javascript/21015393#21015393

@kc-leung using the method I suggested above is solving some of the issues with getting too much space.
So, add these methods to a util/string file

// "vendored" from https://github.com/mdevils/html-entities/blob/68a1a96/src/xml-entities.ts
const decodeXML = (str) => {
  const ALPHA_INDEX = {
    '&lt': '<',
    '&gt': '>',
    '&quot': '"',
    '&apos': '\'',
    '&amp': '&',
    '&lt;': '<',
    '&gt;': '>',
    '&quot;': '"',
    '&apos;': '\'',
    '&amp;': '&',
  }
  if (!str || !str.length) {
    return ''
  }
  return str.replace(/&#?[0-9a-zA-Z]+;?/g, (s) => {
    if (s.charAt(1) === '#') {
      const code = s.charAt(2).toLowerCase() === 'x' ?
        parseInt(s.substr(3), 16) :
        parseInt(s.substr(2))

      if (isNaN(code) || code < -32768 || code > 65535) {
        return ''
      }
      return String.fromCharCode(code)
    }
    return ALPHA_INDEX[s] || s
  })
}

/**
 * https://stackoverflow.com/questions/118241/calculate-text-width-with-javascript/50813259#50813259
 * getTextSize - calculates a rendered text width and height in rem
 * @param { string } text - a text string
 * @param { number || string } fontWeight - text's font weight
 * @param { number } fontSize - text's font size in pixels
 * @param { string } fontFamily - text's font family
 * @returns { object } - the width and height of the rendered text in rem
 */
export const getTextSize = (text, fontWeight, fontSize, fontFamily) => {
  let font = `${fontWeight} ${fontSize}px ${fontFamily}`
  let canvas = document.createElement('canvas')
  let context = canvas.getContext('2d')
  context.font = font
  let metrics = typeof text === 'number'
    ? context.measureText(text)
    : context.measureText(decodeXML(text))
  return {
    width: metrics.width,
  }
}

Then, when you set the width to the TextField do:

 const renderTextfield = () => {
    if (textfieldRef.current) {
      let el = textfieldRef.current.childNodes[0][0]
      el.style.width = `${getTextSize(el.value, 700, 16, 'Open Sans').width}px`
    }
    ...

There seems to be inconsistencies between what input font & size that comes from the tailwindcss. The styling is text-sm, which would translate in 14px font size and a normal font weight, but what we see in reality is thicker and larger. That's why I used

getTextSize(el.value, 700, 16, 'PT Sans')

Please check on this and use the exact values the input field has.

if (textfieldRef.current) {
let el = textfieldRef.current.childNodes[0][0]
const lengthSize = el.value.length > 12 ? 14 : 16
el.style.width = `${getTextSize(el.value, 700, lengthSize, 'Open Sans').width}px`
Copy link
Contributor

Choose a reason for hiding this comment

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

@kc-leung could you determine the font family from the 'el' component and use it instead of Open Sans? I believe widget-studio uses this font in its storybook, otherwise it will probably just use whatever font is in use for the application where it is plugged in. From what I see, the design team uses PT Sans for widget-studio design, which is also in use at the moment in the snoke-dashboard. Open Sans seems to be the fallback font.

body {
  font-family: "PT Sans", "Open Sans", sans-serif;
  ...
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we make the font family for the title Open Sans or PT Sans? sally's figma link is using Open Sans

Copy link
Contributor

Choose a reason for hiding this comment

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

should we make the font family for the title Open Sans or PT Sans? sally's figma link is using Open Sans

@kc-leung Sorry, just saw this. I don't think you should set it to any font family. I believe the Widget Studio will just use whatever font family is in use in the application is plugged in. The only place we setup the font family is for the storybook.
The current Figma design for the Widget Studio uses PT Sans. The dashboard also uses PT Sans as the primary font at the moment.
@hyx131 @YizhiCatherineZhang what do you think?

Copy link
Contributor

@geoerika geoerika Aug 26, 2022

Choose a reason for hiding this comment

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

@kc look how to determine the whole font of the element at the link I previously sent. Try use all the functions there, getCssStyle, getCanvasFont & modify getTextSize to receive only 2 params, text & font.
https://stackoverflow.com/questions/118241/calculate-text-width-with-javascript/21015393#21015393
Screen Shot 2022-08-26 at 4 45 28 PM

Copy link
Contributor

@geoerika geoerika Aug 26, 2022

Choose a reason for hiding this comment

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

Somehow, there are still some inconsistencies with the length calculated here, in WS, which I don't understand where they come from. This function works perfectly for my in chart-system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@geoerika getting dynamically the font-size is not enough since some letters are smaller it is very inconsistent. I rather have it a bit larger than cutting off the title

editable-title - fix outline on focus for input field
@geoerika geoerika changed the title [G2M] replace editable title with borderless textfield [WIP] replace editable title with borderless textfield Sep 15, 2022
@geoerika geoerika added the enhancement New feature or request label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants