Skip to content
This repository has been archived by the owner on Sep 4, 2023. It is now read-only.

Add competence profile Word version #107

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

kallepronk
Copy link
Collaborator

@kallepronk kallepronk commented Jun 22, 2023

I'm pretty much done here, ill keep implementing more stuff unless someone stops me but yeah

@kallepronk kallepronk requested a review from NealGeilen June 22, 2023 14:11
Comment on lines 150 to 158
foreach (var skill in HboIDomain.ProfessionalSkills)
{
var value = skills.First(x => x.skillId == skill.Id);
var cell = CreateTableCellWithBorders("500");
cell.FirstChild?.Append(new Shading
{ Fill = value.maxMastery.Color, });
cell.Append(new Paragraph(new Run(new Text($"{value.count}"))));
valueRow.AppendChild(cell);
}

Check notice

Code scanning / CodeQL

Missed opportunity to use Select

This foreach loop immediately [maps its iteration variable to another variable](1) - consider mapping the sequence explicitly using '.Select(...)'.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is indeed something that can be used

Copy link
Collaborator

@NealGeilen NealGeilen left a comment

Choose a reason for hiding this comment

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

There are some visual changes that need to change.

  • The KPI level legend needs to come first. Not after the tables.
  • The tables are difficult to read because it's a bit cramped and words are cut-off. Personally, I would go for the original style of the component
    image
    There is enough space to show everything.

@NealGeilen NealGeilen marked this pull request as ready for review June 22, 2023 14:28
@Typiqally
Copy link
Owner

Could you povide a screenshot of the Word output?

Comment on lines +18 to +24
body.Append(new Paragraph(new Run(new Text(""))));
body.AppendChild(GetTasks());
body.Append(new Paragraph(new Run(new Text(""))));
body.AppendChild(GetSkills());
body.Append(new Paragraph(new Run(new Text(""))));
body.AppendChild(GetLegend());
body.Append(new Paragraph(new Run(new Text(""))));
Copy link
Owner

Choose a reason for hiding this comment

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

Are these body.Append(new Paragraph(new Run(new Text("")))); nessecary. It seems kind of bloated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If these are not included, all the tables will merge together, will be automatically fixed with the refactor

Copy link
Owner

Choose a reason for hiding this comment

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

Can you atleast replace it with something like a line break?

var headerRow = new TableRow();

// Empty top-left cell.
headerRow.AppendChild(CreateTableCellWithBorders("2500", new Paragraph(new Run(new Text("")))));
Copy link
Owner

Choose a reason for hiding this comment

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

Could you specify what these values are "2500" (maybe make a constant)? Same here with the paragraph stuff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

idk what these values are, still need to figure out what they do

// Set table properties for formatting.
table.AppendChild(new TableProperties(
new TableWidth {
Width = "4", Type = TableWidthUnitValues.Auto, }));
Copy link
Owner

Choose a reason for hiding this comment

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

Expand these to make the code look nices

@Typiqally Typiqally changed the title Feature/word competence profile Add competence profile Word version Jun 27, 2023
@kallepronk
Copy link
Collaborator Author

kallepronk commented Jun 27, 2023

Scherm­afbeelding 2023-06-27 om 18 13 50 Current word output, needs to be changes to Neal's suggestion but that will take a large refactor of this code

@kallepronk kallepronk marked this pull request as draft June 27, 2023 16:15
@Typiqally
Copy link
Owner

The level legend should be under the tables and the "mastered, insufficient, etc." legend can be removed as it is not part of this component. Unless you added it to your screenshot on accident?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants