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

feat(ui5-badge): new property added #8714

Merged
merged 4 commits into from
Apr 23, 2024
Merged

feat(ui5-badge): new property added #8714

merged 4 commits into from
Apr 23, 2024

Conversation

GerganaKremenska
Copy link
Member

No description provided.

@GerganaKremenska GerganaKremenska marked this pull request as draft April 10, 2024 08:25
@ilhan007
Copy link
Member

ilhan007 commented Apr 10, 2024

We have to think about the values Standard, Large look a bit odd, also we tried to align the Avatar and the BusyIndicator like this. I will also take this with me, but please don't merge before we agree on the values.

enum AvatarSize {
	XS = "XS",
	S = "S",
	M = "M",
	L = "L",
	XL = "XL",
}```

and


```ts
enum BusyIndicatorSize {
	S = "S",
	M = "M",
	L = "L",
}```

@GerganaKremenska
Copy link
Member Author

We have to think about the values Standard, Large look a bit odd, also we tried to align the Avatar and the BusyIndicator like this. I will also take this with me, but please don't merge before we agree on the values.

enum AvatarSize {
	XS = "XS",
	S = "S",
	M = "M",
	L = "L",
	XL = "XL",
}```

and


```ts
enum BusyIndicatorSize {
	S = "S",
	M = "M",
	L = "L",
}```

We have to think about the values Standard, Large look a bit odd, also we tried to align the Avatar and the BusyIndicator like this. I will also take this with me, but please don't merge before we agree on the values.

enum AvatarSize {
	XS = "XS",
	S = "S",
	M = "M",
	L = "L",
	XL = "XL",
}```

and


```ts
enum BusyIndicatorSize {
	S = "S",
	M = "M",
	L = "L",
}```

this is only a working version we will have a discussion to decide on property values.

@GerganaKremenska GerganaKremenska marked this pull request as ready for review April 16, 2024 08:14
@@ -0,0 +1,19 @@
/**
* Defines badge size types.

Choose a reason for hiding this comment

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

"size types" is not the best wording. Maybe:
Defines badge size options.
or
Predefined sizes for the badge.

Copy link
Member Author

Choose a reason for hiding this comment

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

"Predefined sizes for the badge". sounds good to me

*/
enum BadgeSize {
/**
* Standard size of the badge

Choose a reason for hiding this comment

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

Pls. double check. "Standard" is OK, but implies that it should always be the default value.

Copy link
Member Author

Choose a reason for hiding this comment

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

"S" is for small, this is old wording.

@BorisDafov
Copy link

Form UA perspective I approve the PR...

@GerganaKremenska GerganaKremenska merged commit a60c5ee into main Apr 23, 2024
9 checks passed
@GerganaKremenska GerganaKremenska deleted the badge_large branch April 23, 2024 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants