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

feat: Truncate tags in UI when they are super long #513

Merged
merged 1 commit into from
Feb 21, 2022

Conversation

bangn
Copy link
Contributor

@bangn bangn commented Oct 6, 2021

No description provided.

@@ -254,7 +256,7 @@ def base_url
def pact_tags
@relationship.tag_names.map do |tag|
{
name: tag,
name: tag.ellipsisize,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Truncate it in the server and let the UI render it.
Another option is to truncate it in UI using JS.
My pref is to truncate at server side to keep it consistent on how tags are currently handled/rendered.
How do you reckon?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. This is the spot that returns the tag names though.

def consumer_version_latest_tag_names
That's code is for the menu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always miss something in my PR :face_palm:. Will make a change. Thanks for pointing out Beth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Please have another look when you have time :D

@bangn
Copy link
Contributor Author

bangn commented Oct 6, 2021

This is to address issue #397
The build are failed but seems like it is not related to my change? 🤔

@bethesque
Copy link
Member

That bit is looking much better now.

Screen Shot 2021-10-08 at 6 42 42 am

Could you apply the same to this screen:

Screen Shot 2021-10-08 at 6 43 27 am

And this screen?

Screen Shot 2021-10-08 at 6 44 01 am

We truncate the version number already, and provide a "copy" button next to it so that the user can easily get the full number to paste into other tools if they need to. Could you have a look at doing that? Maybe the ellipsisize should be called in the HAML, because then you'll have the full value to make the copy button with, and the truncated value.

@bethesque
Copy link
Member

I've fixed the mysql tests, btw, if you want to merge from master.

@bangn
Copy link
Contributor Author

bangn commented Nov 3, 2021

Could you have a look at doing that? Maybe the ellipsisize should be called in the HAML, because then you'll have the full value to make the copy button with, and the truncated value.

That sounds good. I'll do it that way then

@bethesque
Copy link
Member

Looking good. Strange those tests are failing though.

@bethesque
Copy link
Member

Just checking you're not waiting on me for anything. Looking forward to merging this when you've got the tests passing.

@bangn
Copy link
Contributor Author

bangn commented Nov 13, 2021

Hi Beth sorry I was busy lately. Will look into it next week 🙂

@bangn bangn force-pushed the issue-397 branch 2 times, most recently from 3cbf433 to 6fe4073 Compare November 15, 2021 10:38
@bangn
Copy link
Contributor Author

bangn commented Nov 15, 2021

Hey Beth, I have fixed the spec and squash the commits into one. Please let me know if you need me to do anything else

@bangn bangn force-pushed the issue-397 branch 2 times, most recently from e156504 to fdff54f Compare November 15, 2021 10:44
@bethesque
Copy link
Member

Awesome. I'll pick it up on my next oss day.

@bethesque
Copy link
Member

My next OSS day, which I have not had for what seems like months. So sorry this hasn't been merged yet.

@bangn
Copy link
Contributor Author

bangn commented Dec 17, 2021 via email

@bethesque bethesque merged commit 94bbf91 into pact-foundation:master Feb 21, 2022
@bethesque
Copy link
Member

I'm so sorry it's taken me this long to merge. I'm so appreciative of this though!

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.

2 participants