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

Add display text for external medium links #480

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

Splines
Copy link
Member

@Splines Splines commented Apr 27, 2023

Closes #479

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    new feature, see Different representation of external medium links #479 for more details

  • Please check if the PR fulfills these requirements

    • E2E Tests for the changes have been added via Cypress
    • Meaningful rspec tests have been added
    • Docs have been added / updated
  • What is the current behavior? (You can also link to an open issue here)

  • What is the new behavior (if this is a feature change)?

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

  • Other information:

Preview

image

image

image

@Splines Splines self-assigned this Apr 27, 2023
@Splines Splines marked this pull request as ready for review April 27, 2023 13:51
@Splines Splines changed the title Add display text for extern medium links Add display text for external medium links Apr 27, 2023
@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Merging #480 (c871457) into mampf-next (ab2b7b1) will decrease coverage by 0.02%.
The diff coverage is 33.33%.

@@              Coverage Diff               @@
##           mampf-next     #480      +/-   ##
==============================================
- Coverage       66.65%   66.64%   -0.02%     
==============================================
  Files             311      311              
  Lines            9360     9363       +3     
==============================================
+ Hits             6239     6240       +1     
- Misses           3121     3123       +2     
Impacted Files Coverage Δ
app/controllers/media_controller.rb 26.00% <ø> (ø)
app/helpers/media_helper.rb 32.89% <33.33%> (+0.01%) ⬆️

@@ -5,9 +5,10 @@
</h5>
</div>
<div class="card-body text-center">
<%= link_to t('click_here'),
<%= link_to external_link_description_not_empty(medium),
medium.external_reference_link,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if it is visually the most pleasing to have a very large button that visually dominates the whole page if the description text is quite long, but I also do not have an alternative idea. Mabye just plain the text and then a simple "Link"-Button?
button

Copy link
Member Author

@Splines Splines Apr 27, 2023

Choose a reason for hiding this comment

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

I thought the display text was just intended for some short text, e.g. "LA Bosch". That's why the input field (to enter the display text) is also just a small field and not a whole expandable box. The word-break: break-all; is therefore just for the edge case, so that the button does not stick out of the outer div. (Note that edge case also means when the display text is empty, then we show the url itself).

While I don't think that this button is overly pretty for long texts, I also don't really like the option of a "Link" button next to the display text as I feel like links should show the thing they point to directly on them and not somewhere else.

@Splines Splines merged commit 263a355 into mampf-next Apr 28, 2023
@Splines Splines deleted the feature/external-link-text branch April 28, 2023 11:57
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.

2 participants