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 misalignment of copy failure retry CTA #4319

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

KshitijThareja
Copy link
Contributor

@KshitijThareja KshitijThareja commented Oct 30, 2023

Summary

Description of the change(s) you made

This PR aims to fix the misalignment of copy failure retry action which is triggered when the copy operation is unsuccessful in copying a node(s).

Manual verification steps performed

To generate copy error state on the frontend, I raised an exception from the backend copy function. The verification steps I performed:

  1. Ran all the tests
  2. Checked if the frontend for the retry button is as expected.

Screenshots

Before:

before

After fixing the issue:

retry

Does this introduce any tech-debt items?

No. It was just a simple frontend CSS fix.


References

Closes #4272

Comments

The CSS fix works fine for all devices.


Contributor's Checklist

PR process:

  • If this is an important user-facing change, PR or related issue the CHANGELOG label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later time
  • If this includes an internal dependency change, a link to the diff is provided
  • The docs label has been added if this introduces a change that needs to be updated in the user docs?
  • If any Python requirements have changed, the updated requirements.txt files also included in this PR
  • Opportunities for using Google Analytics here are noted
  • Migrations are safe for a large db

Studio-specifc:

  • All user-facing strings are translated properly
  • The notranslate class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)
  • All UI components are LTR and RTL compliant
  • Views are organized into pages, components, and layouts directories as described in the docs
  • Users' storage used is recalculated properly on any changes to main tree files
  • If there new ways this uses user data that needs to be factored into our Privacy Policy, it has been noted.

Testing:

  • Code is clean and well-commented
  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Any new interactions have been added to the QA Sheet
  • Critical and brittle code paths are covered by unit tests

Reviewer's Checklist

This section is for reviewers to fill out.

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

Unfortunately, it still looks misaligned to me. A screenshot and line shows it too. There's a possibility that different display densities are affecting this approach.
Untitled drawing

Making use of line-height might help. Setting your change to 2px looks right to me, does that look the same for you?

Untitled drawing (2)

@KshitijThareja
Copy link
Contributor Author

@bjester using line-height won't help in my opinion. I tried setting bottom padding to 2px and 5.5px. I'll show you the results.

With 2px:
2px

With 5.5px
5 5px

I can push the changes with whatever value looks fine to you. I'd prefer the second one personally.

@bjester
Copy link
Member

bjester commented Oct 31, 2023

@bjester using line-height won't help in my opinion. I tried setting bottom padding to 2px and 5.5px.

Changing the line-height of the link button certainly won't help 😏

I can push the changes with whatever value looks fine to you. I'd prefer the second one personally.

The two bits of text should appear as if it's sitting on an imaginary baseline, with the underline underneath that baseline. The 2px option looks closest to that

@KshitijThareja
Copy link
Contributor Author

KshitijThareja commented Oct 31, 2023

Changing the line-height of the link button certainly won't help 😏

Ah, you got me. Sorry for the mistake 😅
Here's the final result:

2px

This should work according to me.

@KshitijThareja
Copy link
Contributor Author

@bjester I have pushed the changes. Thanks for your help with everything : )

@bjester bjester merged commit b3b43ac into learningequality:hotfixes Oct 31, 2023
9 checks passed
@bjester
Copy link
Member

bjester commented Oct 31, 2023

Thanks @KshitijThareja !

@pcenov
Copy link
Member

pcenov commented Mar 8, 2024

Hi @KshitijThareja is there a way for me to manually test and replicate this on hotfixes?

@KshitijThareja
Copy link
Contributor Author

KshitijThareja commented Mar 8, 2024

Hi @pcenov! To replicate this on hotfixes, just raise a validation error inside the try block here:

try:
source = self.get_queryset().get(pk=from_key)
except ContentNode.DoesNotExist:
raise ValidationError("Copy source node does not exist")

Now if you try to copy resources in a channel, you'll be able to replicate the Copy Failure error.

@akolson akolson mentioned this pull request Apr 17, 2024
@bjester bjester added this to the Studio: rebranding patch milestone Apr 24, 2024
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