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

TAN-1529 Strategic report template #8040

Merged

Conversation

luucvanderzee
Copy link
Contributor

Changelog

Copy link

@cl-dev-bot
Copy link
Collaborator

cl-dev-bot commented May 28, 2024

Warnings
⚠️

The changelog is empty. What should I put in the changelog?

⚠️ The PR title contains no Jira issue key (case-sensitive)
⚠️ The branch name contains no Jira issue key (case-sensitive)
Messages
📖

Run the e2e tests

📖 Check translation progress

Generated by 🚫 dangerJS against 953caa8

@luucvanderzee luucvanderzee marked this pull request as ready for review May 28, 2024 16:07
<li>${formatMessage(aboutMessages.projectLabel, {
projectsList: projectTitle?.[locale] ?? '',
})}</li>
<li>
Copy link
Contributor Author

@luucvanderzee luucvanderzee May 28, 2024

Choose a reason for hiding this comment

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

Making this bold using <b> tags for consistency with Platform template

Copy link
Contributor

@adessy adessy left a comment

Choose a reason for hiding this comment

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

Good to go!

Not directly related to this PR, but I'm wondering if we coul reduce repetitions in the design of some widgets when showing comparisons. Take for instance "Previous ... days" in the methods widget. This should in principle always be the same comparison for all the methods, so repeating it does not seem necessary. We have this issues with other widgets, but it's most noticeable here.

image

If we don't want to change the design now, I would at least try to tweak the width of the method box to fit two rows of 4 methods (since the number of methods is fixed and we cannot toggle them atm) and prevent the comparison line from wrapping.

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 update the copy for messages.createReportModalTitle.

Copy link
Contributor

Choose a reason for hiding this comment

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

(it would be nice to be able to confirm model with shift+enter)

const [selectedProjectId, setSelectedProjectId] = useState<
string | undefined
>();
const [dates, setDates] = useState<Dates>({ startDate: null, endDate: null });

const [errorMessage, setErrorMessage] = useState<string | undefined>();
const { formatMessage } = useIntl();
const reportTitleTooShort = reportTitle.length <= 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering why we have this restriction. If the customer wants to use sh**** names, they should be allowed to. (Also, we don't do that kind of check when renaming and it's not enforced by the BE either.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idk, I think it would be weird to have reports that have a zero length name. So I think we should at least enforce a one character limit. But then why not two?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we definitely should not allow empty strings, but I think names with a length of 1 should be allowed. It's like naming files on your computer, it's up to the user to pick good names.

@luucvanderzee
Copy link
Contributor Author

luucvanderzee commented Jun 4, 2024

Also not super happy with the design, but not sure what to do to improve it. Fixing it can be a purely graphical change so it should be easy to fix later.

I would at least try to tweak the width of the method box to fit two rows of 4 methods (since the number of methods is fixed and we cannot toggle them atm)

I can try this, but the widget will also be available for other screen sizes so this would of course only work for A4

and prevent the comparison line from wrapping.

It might still look bad in other languages though...

@luucvanderzee luucvanderzee merged commit 8904685 into TAN-1525-projects-widget Jun 4, 2024
@luucvanderzee luucvanderzee deleted the TAN-1529-strategic-report-template branch June 4, 2024 12:21
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.

None yet

3 participants