Skip to content

Conversation

@droshev
Copy link
Contributor

@droshev droshev commented Jan 5, 2026

signalify skeleton and update jsdoc

@droshev droshev requested a review from a team January 5, 2026 02:26
@droshev droshev self-assigned this Jan 5, 2026
@droshev droshev added enhancement New feature or request documentation There is an issue with documentation labels Jan 5, 2026
@droshev droshev added this to the Sprint 154 - January 2026 milestone Jan 5, 2026
@netlify
Copy link

netlify bot commented Jan 5, 2026

Deploy Preview for fundamental-ngx ready!

Name Link
🔨 Latest commit ca208e4
🔍 Latest deploy log https://app.netlify.com/projects/fundamental-ngx/deploys/695b21346eee900008963467
😎 Deploy Preview https://deploy-preview-13699--fundamental-ngx.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

* Unique ID for SVG mask element.
* @hidden
*/
protected readonly _id = `fd-skeleton-${skeletonUniqueId++}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a private attr and should not be prefixed with underscore (it's used in the template).

* Last line is 60% width when there are multiple lines.
* @hidden
*/
protected readonly _textLineWidths = computed(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

same for this one. It's used in the template. We prefix with underscore only private properties.

@InnaAtanasova InnaAtanasova mentioned this pull request Jan 5, 2026
5 tasks
const currentHeight = this.height();

if (currentType === 'text') {
const lines = this.textLines() || 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

This fallback to 1 should never be needed since textLines has a default value set to 3

const lines = this.textLines() || 1;
return {
width: currentWidth ?? '100%',
height: currentHeight ?? (lines > 1 ? `${20 * lines}px` : '8px')
Copy link
Contributor

Choose a reason for hiding this comment

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

This magic number 20 can be a protected member of the class, say LINE_HEIGHT, which can also be used in the template (line 19).

@MariaIDineva
Copy link
Contributor

MariaIDineva commented Jan 6, 2026

Same comment as the one in title component PR. Maybe we sould replace the import of the module with the component import since we should encourage standalone now and we have deprecated the module.
Screenshot 2026-01-06 at 10 40 14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation There is an issue with documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants