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

Fix [datahub]: Display external viewer button correctly #656

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

tkohr
Copy link
Collaborator

@tkohr tkohr commented Oct 20, 2023

Fixes the external viewer button display while allowing truncating long values in the dropdown select.

button
no-button

Copy link
Member

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

Does it works well on mobile too ?

Thanks you 🙏

@github-actions
Copy link
Contributor

github-actions bot commented Oct 20, 2023

Affected libs: feature-record, feature-router, ui-inputs, feature-dataviz, feature-search, feature-map, feature-editor, ui-elements, feature-catalog, ui-catalog, ui-search,
Affected apps: metadata-editor, datahub, demo, webcomponents, metadata-converter, search, map-viewer, datafeeder,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@fgravin fgravin added the bug Something isn't working label Oct 20, 2023
@fgravin fgravin added this to the 2.0.1 milestone Oct 20, 2023
@tkohr
Copy link
Collaborator Author

tkohr commented Oct 20, 2023

Does it works well on mobile too ?

Ah, I forgot mobile, I'll have a look

@coveralls
Copy link

coveralls commented Oct 20, 2023

Coverage Status

coverage: 81.806% (-4.7%) from 86.461% when pulling 1f70384 on fix-external-button-flex into 3c899e0 on main.

template allows truncate in dropdown within flex layout, without cutting outlines
@tkohr tkohr force-pushed the fix-external-button-flex branch from fa83aac to adeabd3 Compare October 20, 2023 10:06
@tkohr
Copy link
Collaborator Author

tkohr commented Oct 20, 2023

I've added the place-self-end for correct positioning of the button on mobile.

Independent of this PR the truncate within the dropdown-selector is broken when switching to mobile layout and applying flex-col here. I remember we already had troubles making truncate work within flex in #516.

For flex-row this works, but apparently not when the child switches to flex-col.

<gn-ui-dropdown-selector
class="truncate p-1 -mx-1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised that this does anything; for me the dropdown button already has a truncate built-in if the width is too low for the text to show

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, strangely this is necessary, because of the flex I added in the parent.

@tkohr
Copy link
Collaborator Author

tkohr commented Oct 23, 2023

I've added two more commits to fix the dropdown truncate when the title is displayed on top of the dropdown, as well the currently broken truncate in the data-view.component due to the flex around the dropdown.

mobile-no-button mobile-button

Everything displays well on desktop and mobile on my side now.

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

It looks good, I wish we could understand why this kind of class is needed outside of the component (it should be able to decide its own size correctly), but I looked and couldn't find any reason so, let's go with this.

Thanks @tkohr!

@tkohr tkohr merged commit 8b7916f into main Oct 24, 2023
5 checks passed
@tkohr tkohr deleted the fix-external-button-flex branch October 24, 2023 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants