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

Adjustments according to new no data page layout #691

Merged
merged 7 commits into from
Sep 29, 2023
Merged

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Sep 18, 2023

Description:

The structure of the Tracking Code components is currently very complex. Tried to simplify that a bit, but it's still not possible to use the components to receive a clean html list structure. There are always div elements that aren't part of a li, or li elements that are surrounded by div. The list for tag manager currently doesn't use the new global list style for decimal lists, as it only works with a clean html list structure. I guess for that the components would needed to be fully refactored...

refs matomo-org/matomo#21247

Review

@sgiehl sgiehl marked this pull request as ready for review September 26, 2023 11:56
Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

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

Looks good overall. I think the alignment of the note in step 1 of the MTM tab isn't consistent with the GA tab:
image
image

From speaking to Product during previous tickets, I believe that the GA tab has the desired alignment.

@sgiehl
Copy link
Member Author

sgiehl commented Sep 28, 2023

@snake14 I'm aware that the align in that list isn't as beautiful as in the others. The problem is (as mentioned in the PR description) that the structure of the TagManager components results in a html structure that does not look like

<ul>
  <li>...</li>
  <li>...</li>
</ul>

but contains nested levels like

<ul>
  <li>...</li>
  <div>
    <li>...</li>
  </div>
</ul>

That makes it impossible to use the global layout as it requires a simple structure to render correctly.
I tried to refactor the components, but gave up at some points as the code was too complex to understand in a short time.

Btw. the list layout as it was use before didn't work correctly at all. It tried to intend all following lines using a padding or margin. That only works as long as the text fits in one line. Which already wasn't the case in a lot translations.
So imho it's better to not indent it here at all for now and wait till the components were refactored in a way that a clean html structure is returned.

@snake14
Copy link
Contributor

snake14 commented Sep 28, 2023

@sgiehl I don't think there are any plans to refactor this page as it was recently redesigned. Part of the problem is that the components are used in multiple places, plus using components means that there are some unexpected divs inside of lists and we had to account for that. As far as the note container I mentioned, we simply indented that container (div or p) and let all the others follow their default wrapping behaviour.

@michalkleiner
Copy link
Contributor

Refactor to remove tech debt/improve the markup shouldn't be based on how recently the page has been redesigned. Even components that have extra divs should be able to be put in lists and respect their styling, e.g. by having the div inside and not outside of the div and so on. Or use <template> tags in vue components so that they don't produce additional markup where not needed.

Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

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

I think it would be good to try to indent the note so that it matches the GA and GTM tabs, but it sounds like more work is needed in this tab, so we're probably fine merging this PR as it is.

@sgiehl
Copy link
Member Author

sgiehl commented Sep 29, 2023

I'm merging this one for now. We can add further improvements later

@sgiehl sgiehl merged commit 48bbd90 into 5.x-dev Sep 29, 2023
3 of 4 checks passed
@sgiehl sgiehl deleted the nodatalayout branch September 29, 2023 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants