Skip to content

feat: added ht-status-display component #1775

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: added ht-status-display component #1775

wants to merge 1 commit into from

Conversation

chahat99
Copy link
Contributor

@chahat99 chahat99 commented Aug 5, 2022

Added ht-status-display component

@chahat99 chahat99 requested a review from a team as a code owner August 5, 2022 07:24
@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

Merging #1775 (3f08e90) into main (0eeceb0) will decrease coverage by 0.01%.
The diff coverage is 71.87%.

@@            Coverage Diff             @@
##             main    #1775      +/-   ##
==========================================
- Coverage   83.58%   83.56%   -0.02%     
==========================================
  Files         893      897       +4     
  Lines       19259    19291      +32     
  Branches     2663     2667       +4     
==========================================
+ Hits        16098    16121      +23     
- Misses       3024     3033       +9     
  Partials      137      137              
Impacted Files Coverage Δ
...components/status-display/status-display.module.ts 0.00% <0.00%> (ø)
...ponents/status-display/status-display-icon.pipe.ts 90.00% <90.00%> (ø)
...ponents/status-display/status-display-text.pipe.ts 100.00% <100.00%> (ø)
...ponents/status-display/status-display.component.ts 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@github-actions
Copy link

github-actions bot commented Aug 5, 2022

Unit Test Results

       4 files     300 suites   23m 59s ⏱️
1 068 tests 1 068 ✔️ 0 💤 0 ❌
1 077 runs  1 077 ✔️ 0 💤 0 ❌

Results for commit 3f08e90.

@Input()
public statusMessage?: string;

// TODO: Add display for different styles if required
Copy link
Contributor

Choose a reason for hiding this comment

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

@arjunlalb @adisreyaj
We have this component as gray background and since we want this to be used at the status table cell renderer (which has different styling according to the status), how do we wanna approach this?

  1. Should we replace that styling with this new one
  2. Or add that styling as a config here as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should use a single style. I recommend using the existing style. After consulting with design team, we can switch it over to the new one or keep the old one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree on keeping the same styling throughout the product and yeah, the existing one seems better to me as well. But let's wait for @adisreyaj to comment on this since I think he has more context around that.

I think, We can leave this here as it is until we get final confirmation.

@itssharmasandeep
Copy link
Contributor

Please export this as well from public-api.ts file present in the components project

*/
public transform(statusCode: string | number, statusMessage?: string): string {
let statusDisplayText = displayString(statusCode);
if (!isNil(statusMessage) && statusMessage !== 'null') {
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we looking for string null here?

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.

4 participants