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

Ttonev/hr portal #3

Open
wants to merge 14 commits into
base: vnext
Choose a base branch
from
Open

Ttonev/hr portal #3

wants to merge 14 commits into from

Conversation

tishko0
Copy link

@tishko0 tishko0 commented Mar 20, 2025

No description provided.

@tishko0 tishko0 requested a review from dkamburov March 20, 2025 10:02
@dkamburov dkamburov requested a review from mddragnev March 20, 2025 12:20
@MayaKirova
Copy link

MayaKirova commented Mar 24, 2025

  • There are some minor difference between the angular toolbar and this one.

    Here's a comparison, angular on the right, wc on the left.
    image

    Notice that:

    • Title "HR Portal" is not aligned to the left in the wc one.
    • Button text in the toolbar is darker in the Angular one, compared to the wc one.
  • Also after sorting a column, in angular a new button for clearing sort shows. In wc it does not show:

image

  • If we compare both the angular and wc sample with the figma design:

    • Title in design is "Actions", while in the sample it is "HR Portal"
    • Column "Name" is initially sorted in the design, but not in the samples.

    Either the samples or the design should be updated so that there are no differences.

  • Noticed that there are also some console.logs left over. Those should be removed:
    image

@MayaKirova MayaKirova added ✅ status: verified Applies to PRs that have passed manual verification 🛠️ status: in-development Issues and PRs with active development on them and removed ✅ status: verified Applies to PRs that have passed manual verification labels Mar 24, 2025
Copy link
Member

Choose a reason for hiding this comment

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

This should not be in the public folder. All things in public folder are served at the root path. This is not needed. This better be in src/assets

}

async fetchData() {
try {
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this to a separate service file so that it would be similar as the other samples


async fetchData() {
try {
const response = await fetch('https://staging7.infragistics.com/grid-examples-data/data/hr/hr.json');
Copy link
Member

Choose a reason for hiding this comment

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

move that url to a constant var

}

clearSorting() {
const treeGrid = this.shadowRoot?.getElementById('treeGrid') as IgcTreeGridComponent;
Copy link
Member

Choose a reason for hiding this comment

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

You can use the @query decorator to get a referance to the grid

}

public avatarTemplate = (ctx: IgcCellTemplateContext) => {
let row = ctx.cell.row;
Copy link
Member

Choose a reason for hiding this comment

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

isnt this the same as ctx.cell.value? why get the value from the row. Same as all other body templates


body {
display: flex;
font-family: 'Helvetica', sans-serif;
Copy link
Member

Choose a reason for hiding this comment

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

you can set ig-scrollbar ig-typography these two classes to the body element in the index.html and you wont need to set font-family here. Or even you insist on setting font-family here please use the corresponding --ig-font-family var from the theme

color: #535bf2;
}
igc-tree-grid::part(row) {
background-color: #ffffff(--row-even-background);
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not used. Also, it is not a correct value for background-color

margin: 0 auto;
padding: 2rem;
align-items: center;
Copy link
Member

Choose a reason for hiding this comment

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

If this is gonna be a block container, then you dont need align-items, justify-content and gap. Probably, wont need the margin and text-allign as well

}

app-hr-portal {
flex: 1;
Copy link
Member

Choose a reason for hiding this comment

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

These override the :host styles that you set in the hr-portal.ts file. If you dont need this, please remove it. Also, why flex on a host that has only 1 element?

Copy link
Member

Choose a reason for hiding this comment

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

Apply some formatting to the file as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛠️ status: in-development Issues and PRs with active development on them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants