-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Status, Status Link Controls update #1580
Status, Status Link Controls update #1580
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't commit reformated files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to commit a file if it is only reformated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please advise on the next steps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'll have to modify it to test the new property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we allow the choice between letter and icon, for example add a property with_icon
boolean True by default.
What do you think @FlorianJacta @FabienLelaquais ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, we should
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please advise on the next steps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of advice are you looking for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- add a property to taipy\gui_renderers\factory.py L 494 :
("with_icon", PropertyType.boolean, False),
- add the corresponding property to frontend\taipy-gui\src\components\Taipy\StatusList.tsx L 88:
withIcon?: boolean;
- declare a default value L92:
withIcon = false
- pass the value to the Status Component L 170 and L 161 : withIcon={withIcon}
- in frontend\taipy-gui\src\components\Taipy\Status.tsx declare and use the
withIcon
- add tests to StatusList and Status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pravintargaryen @FredLL-Avaiga @FlorianJacta
Before a proposal for implementation is done, I would like to have some requirements.
I really don't like the with_icon
proposal that addresses only the issue (which is good) but makes little sense for future improvements.
This 'all or nothing' approach sounds really bad to me. We should let users decide what to show for what status priority.
Maybe some application has a special need where a new status priority would be useful as well (say the application enters a disastrous mode, where users would want to create a status line like status = ("disaster", "Out of memory.")
.
Each status priority could have an associated icon, accessed in a dictionary for example.
So the default would be a dict:
icons = {
"S": <success-icon>,
"E": <error-icon>,
"W": <warning-icon>,
"I": <info-icon>
}
and the "question mark" fallback, we would let users have the possibility of customizing the status priority representation by overloading existing ones or adding their own.
My first proposal would then be a new property called icons
, holding something that would be a dict that merges with the predefined one.
- Each key would be a status priority.
When a new status record arrives, we search for a corresponding key: one that, case-insensitively, would best match the recode status priority (today, the rule is 'starts with', which is fine — we can think of something better like 'most matching starting characters'). - Each value would be an Icon object, that we already have all over the place in Taipy.
The result is:
- The status control represents status priorities with a relevant icon by default
- If a user prefers the text version, then the property must be set to a dict that overloads all priority ids with None
- If an application requires a new status priority, it can be decorated using the property.
I'm still not clear on the 'icon selection' rule - but it can be addressed by really listing use cases.
My final point are:
- do not rush implementing features that are unclear or limiting.
- why invent a new type of Icon in Taipy? Or can Icon rely on MaterialUI icons with some sorcellery?
Comments are welcome.
Code, not just yet.
This PR has been labelled as "🥶Waiting for contributor" because it has been inactive for more than 14 days. If you would like to continue working on this PR, then please add new commit or another comment, otherwise this PR will be closed in 14 days. For more information please refer to the contributing guidelines. |
This PR has been labelled as "🥶Waiting for contributor" because it has been inactive for more than 14 days. If you would like to continue working on this PR, then please add new commit or another comment, otherwise this PR will be closed in 14 days. For more information please refer to the contributing guidelines. |
This PR has been closed because it has been marked as "🥶Waiting for contributor" for more than 14 days with no activity. |
Resolves: #1157