feat: add graphics full tab business logic (MIN-165)

Closes MIN-165

Objective:

Implement export graphics tab business logic

What has been done:

• CheckboxFilter handle radio type
• ImageFormat component
• ImageSize component
• graphics download handler
• unit tests for all above

assigned to @AdrianOrlow

added 18 commits

• cbc2243d...459df684 - 17 commits from branch development
• 834e532a - feat: merge changes

Compare with previous version

added 1 commit

• 0dee60b6 - feat: add tests

Compare with previous version

marked this merge request as ready

changed the description

Small inconsistency here. Component is not tested. I don't see that its covered by any test

Component tests added

By looking at Submap functionality - it looks like its more current(selected)Model then models. We can have only one model selected at given time

changed this line in version 4 of the diff

Fix'd

I understand your intention of unit testing hooks (because of pair programming) - thats why this helper exist. I'm still not huge fan of writing big utilities to allow us to test single function cuz it all could be covered in component tests. Approach to testing like this could be part of team discussion in the future - I see it as opportunity to learn.

I agree - that's a good point for discussion

It might be good idea to move it to apiPath.ts. This way we would have all links in one place in code base

It's not a good idea as some properties may be falsy, so that would cause some chaos in apiPath

I feel I miss some documentation here. I do understand the code but I don't understand reasons why this work this way.

Done

How do you know all components work together to trigger download graphics correctly? If I remove image size component - will functionality still work?

Every element is needed, so if any is lacking, data fetch will simply not start

export const getGraphicsDownloadUrl = ({
  backgroundId,
  modelId,
  handler,
  zoom,
}: GetGraphicsDownloadUrlProps): string | undefined => {
  const isAllElementsTruthy = [backgroundId, modelId, handler, zoom].reduce(
    (a, b) => Boolean(a) && Boolean(b),
    true,
  );

thats strange. You take every format as argument which is always truthy (by inspecting the type) and check if it's truthy? It's probably some TS error here hidden

changed this line in version 4 of the diff

Yep, that's a weird error and I don't understand reason behind these changes. I based on some other selector and probably missed something

I think it would be simpler to create a Map (you won't need filtering empty values in line 106) and then pick values you need in imageHandlersSelector

changed this line in version 4 of the diff

Yep, that would be a better solution but as you said in another comment, there's no need for filtering

It could be roundNumberToInteger or floatToInteger. If you want to emphasize that it handles NaN or 0 it could be safeNumberToInteger

changed this line in version 4 of the diff

NaN and 0 are also numbers

I agree with proposition of adding safe keyword to name. The new name is numberToSafeInt

LGTM. Pair programming session definitely helped me understand code faster. It's clear to see that choosing compound patter didn't work well since submap and image size component are tight together logically.

approved this merge request

I noticed one more thing. I would add margin at the bottom to add some space just bellow download button. It could look a bit better to the user

added 1 commit

• 359c31b8 - feat: add rfc changes

Compare with previous version

merged

mentioned in commit 29618c1f