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

kie-issues#1527: DMN Editor: Create new overlay option 'Highlight evaluation results' #2657

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jomarko
Copy link
Contributor

@jomarko jomarko commented Oct 11, 2024

@jomarko jomarko added area:dmn lang:typescript Pull requests that update Javascript code labels Oct 11, 2024
@jomarko jomarko changed the title kie-issues#1527: DMN Editor: Create new overlay option 'Highlight eva… kie-issues#1527: DMN Editor: Create new overlay option 'Highlight evaluation results' Oct 11, 2024
@jomarko jomarko marked this pull request as ready for review October 14, 2024 09:41

export function EvaluationHighlightsBadge() {
const dmnEditorStoreApi = useDmnEditorStoreApi();
const isEvaluationHighlights = useDmnEditorStore((s) => s.diagram.overlays.enableEvaluationHighlights);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const isEvaluationHighlights = useDmnEditorStore((s) => s.diagram.overlays.enableEvaluationHighlights);
const isEvaluationHighlightsEnabled = useDmnEditorStore((s) => s.diagram.overlays.enableEvaluationHighlights);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you @tiagobento, I agree, it is a better name, changes pushed

@tiagobento
Copy link
Contributor

Other than that ^ looks good to go! :)

@tiagobento
Copy link
Contributor

image

Border looks a little off, and the tooltip says (beta), but it isn't.

Also, I think the idea of having this as a https://www.patternfly.org/components/label/ instead of a "Button" is important to give it a different look compared to the other buttons, since this this is not really a button, but more like an indicator that the Evaluation highlights are turned on.

Same is true for the toggle on BEE.

We also need to be mindful of the case we choose when writing text on-screen. I always prefer to capitalize only the first word, not all words, so the label would read "Evaluation highlights: on/off", instead of "Evaluation Highlights: On/Off".

Another thing that caught my eye was the little displacement that happened when the I toggled between on and off on BEE, because of difference in text size. Being mindful of layout shifts is also very important for a good experience when using a web app.

Please consider my feedback, and let me know if you have questions!

@jomarko
Copy link
Contributor Author

jomarko commented Oct 18, 2024

@tiagobento good feedback, I will try to incorporate it.

Border looks a little off, and the tooltip says (beta), but it isn't.

I will remove the "(beta)". Oh, you are correct, I haven't noticed the border off placement. It is an example that author eyes are focused on other things than reviewer eyes. Thank you for spotting.

Also, I think the idea of having this as a https://www.patternfly.org/components/label/ instead of a "Button" is important to give it a different look compared to the other buttons, since this this is not really a button, but more like an indicator that the Evaluation highlights are turned on.

Ok, probably this I didn't understand from mockup. There it seemed to me as UI element with same semantic as other elements in that area, that is why I used the button. I will incorporate your proposal.

Same is true for the toggle on BEE.

Sure.

We also need to be mindful of the case we choose when writing text on-screen. I always prefer to capitalize only the first word, not all words, so the label would read "Evaluation highlights: on/off", instead of "Evaluation Highlights: On/Off".

I do not have preference about this. I don't mind follow your suggestion.

Another thing that caught my eye was the little displacement that happened when the I toggled between on and off on BEE, because of difference in text size. Being mindful of layout shifts is also very important for a good experience when using a web app.

WIll try to fix the size of the new toggle/label. However, that will probably won't work with potential i18n in future.

@jomarko
Copy link
Contributor Author

jomarko commented Oct 18, 2024

Changes pushed:
Screenshot 2024-10-18 121225
Screenshot 2024-10-18 121220
Screenshot 2024-10-18 121209

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dmn lang:typescript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DMN Editor: Create new overlay option “Highlight evaluation results”
2 participants