Skip to content

Ttonev/hr portal latest #6

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

Merged
merged 21 commits into from
Apr 25, 2025
Merged

Ttonev/hr portal latest #6

merged 21 commits into from
Apr 25, 2025

Conversation

tishko0
Copy link
Contributor

@tishko0 tishko0 commented Apr 10, 2025

No description provided.

@tishko0 tishko0 requested a review from dkamburov April 10, 2025 11:30
@tishko0 tishko0 added the ❌ status: awaiting-test PRs awaiting manual verification label Apr 10, 2025
@skrustev skrustev changed the base branch from vnext to skrastev/sales-tabs April 14, 2025 07:47
Copy link
Member

@mddragnev mddragnev left a comment

Choose a reason for hiding this comment

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

  1. Could you please remove the routing from the app. We dont need all that boilerplate. Also, delete the files that you dont need.
  2. When running npm run build an error is thrown

@MayaKirova
Copy link

When building and running the main app( npm run build and npm run preview), routing to the sample seems to be incorrect as it currently loads the wrong view from the hr-portal-view and not the actual sample:
image

@tishko0
Copy link
Contributor Author

tishko0 commented Apr 22, 2025

  1. Could you please remove the routing from the app. We dont need all that boilerplate. Also, delete the files that you dont need.
  2. When running npm run build an error is thrown

it is building for me, can you please show what error it throws:
image

If it is regarding the main app as per maya's comment, I have updated the routes for the hr-portal, the rest of the apps still point towards the view tho

@mddragnev
Copy link
Member

Please inside the project/hr-portal remove the routing since it is not needed.

Base automatically changed from skrastev/sales-tabs to vnext April 23, 2025 11:39
@tishko0
Copy link
Contributor Author

tishko0 commented Apr 23, 2025

I had to make some changes to the sales grid in order for the build not to fail, not sure but the sales branch was not failing as it was, or i have warnings to pop as errors.

@tishko0 tishko0 requested a review from mddragnev April 24, 2025 09:24
@tishko0 tishko0 requested a review from mddragnev April 24, 2025 13:58
@@ -0,0 +1,59 @@
body {
margin: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need these styles for margin of the body both here and in styles.scss?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just one element so I removed the body completely

const gridRef = useRef<IgrTreeGrid | null>(null);

useEffect(() => {
dataService.fetchData().then((fetchedData: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please introduce some data types and use them both here, in the service and wherever needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i created an interface in the data service and imported it from there. If you would like it to be in the main app please let me know

};

const dateTemplate = ({ cell }: IgrCellTemplateContext) => {
const data = cell.row.data;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use cell.row.data here? You only need the value of the column here formatted in some way. You can remove the { } around cell and use .cell.value instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was for consistency with the rest of the templates, but if you think this way is better i am with you so I have changed it

@MarielaTihova MarielaTihova added ✅ status: verified Applies to PRs that have passed manual verification ❌ status: not-fixed and removed ❌ status: awaiting-test PRs awaiting manual verification ❌ status: not-fixed labels Apr 24, 2025
@dkamburov dkamburov merged commit 5e5520f into vnext Apr 25, 2025
1 check passed
@dkamburov dkamburov deleted the ttonev/hr-portal-latest branch April 25, 2025 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ status: verified Applies to PRs that have passed manual verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants