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

[BE][HUD] Introduce Label to GroupHudTable #5876

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

[BE][HUD] Introduce Label to GroupHudTable #5876

wants to merge 29 commits into from

Conversation

yangw-dev
Copy link
Contributor

@yangw-dev yangw-dev commented Nov 7, 2024

Overview

Add label guide and label for jobs in GroupHudTable

Background

Currently we use simple text ("Q","S","~" etc) to indicate the state of git jobs, it does not make important indicators to stand out, for instance, the success test should has less vision impact compared to other signals.

Details

  • Replace most of the indicators with react-icon

OSS license

By following guide [Third_Party_in_OSS(https://www.internalfb.com/intern/wiki/Open_Source/Contribute_to_FB_OSS_Project/Third_Party_in_OSS/) All react-icon libs we use in this PR from react-icons are under approved license. One thing notice that, in REACT-ICONs the library Weather Icons's license SIL OFL 1.1 is not in the list Weather Icons SIL OFL 1.1

Do NOT MERGE UNTIL

the dependency fix kicked in #5887 for package.json update

Copy link

vercel bot commented Nov 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
torchci ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 9, 2024 1:25am

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 7, 2024
@yangw-dev yangw-dev changed the title [HUD] Introduce Label to GroupHudTable [BE][HUD] Introduce Label to GroupHudTable Nov 7, 2024
@yangw-dev
Copy link
Contributor Author

yangw-dev commented Nov 8, 2024

the floating feature for label guide can be disabled, can simply put it on left above the table. open for discussion

@yangw-dev
Copy link
Contributor Author

yangw-dev commented Nov 8, 2024

there are some logic rendering on time (for instance the flaky test is failure with green color), maybe someone can list me the label condition I missed, so i can add to the lable guide @clee2000 @huydhn

</Item>
);
}
export default LabelGuide;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you decide to keep the guide, I think it would be better to have things horizontally aligned? Like the all null ~ and the not exist ~ should both be on the same row. Idk how hard that would be tho
image

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, I don't think we really need a legend table as most of the icons are easy to recognized. Even if there is one, I would prefer it to be collapsed by default. I think people will be confused about too much details like all null, not exist, neutral, warning only.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the label is already part of the job/group description when you hover the mouse over, so the legend table becomes redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! I can remove the label guide

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we still have a blinking icon for job that is in progress?
Also, as both in-progress and queue are represented by similar icon,I wonder if gray clock could indicate that jobs is queued, while yellow blinking one means that it's in progress...

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't recall ever having a blinking icon for in progress jobs?

Copy link
Contributor Author

@yangw-dev yangw-dev Nov 8, 2024

Choose a reason for hiding this comment

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

@malfet you mean like this lol? (Dark mode)
shiningPending
I changed it to rotating, let me know if this is better:

rotating

@clee2000
Copy link
Contributor

clee2000 commented Nov 8, 2024

I think there's some unintended behavior on the pr/commit pages
image

@huydhn
Copy link
Contributor

huydhn commented Nov 8, 2024

I wonder if there is a better icon for flaky tests, F is actually clearer IMO, the new icon looks unfamiliar, but maybe it's just me. So, it might be ok

@huydhn
Copy link
Contributor

huydhn commented Nov 8, 2024

Also please test it out on dark mode to see if it looks ok, I'm pretty sure that some people are using that night-owl mode :P

@yangw-dev
Copy link
Contributor Author

chatted offline with catherine, will make pending less stand out

@yangw-dev
Copy link
Contributor Author

mind tell me more what is unitended behaviour? if you mean the icon is not inline with the word, that can be fix in next pr, since it needs to update to other pages too

I think the side effect of breaking up the icon and the job name is that HUD page becomes longer to scroll. IIRC, people didn't like that and asked for a more compact form

I see, I can fix that after this PR, meanwhile Catherine made good point that i may also have some feed back from treehugger about this icon changes, see what people think about this

Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

Do you mind landing module updates and file moves as part of separate PRs?

torchci/package.json Outdated Show resolved Hide resolved
torchci/components/JobConclusion.module.css Outdated Show resolved Hide resolved
@yangw-dev yangw-dev changed the base branch from main to adddependency November 9, 2024 01:16
@yangw-dev yangw-dev changed the base branch from adddependency to main November 9, 2024 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants