Skip to content

Conversation

@filipemarins
Copy link
Contributor

@filipemarins filipemarins commented Aug 30, 2022

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Go to Administration > Settings > Assets is not showing the setting description

Further comments

What:
Remove validation to check if the translation key exists to fulfill the settings label.

Why:
After a typescript rewrite was added validation to check if translation key exists, but some settings label doesn't have a translation key, in this case, the label value was undefined.

@filipemarins filipemarins requested review from a team as code owners August 30, 2022 21:16
@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #26755 (d77e076) into develop (18b7b4b) will increase coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #26755      +/-   ##
===========================================
+ Coverage    40.61%   40.81%   +0.19%     
===========================================
  Files          790      790              
  Lines        17903    17903              
  Branches      1927     1927              
===========================================
+ Hits          7272     7307      +35     
+ Misses       10342    10300      -42     
- Partials       289      296       +7     
Flag Coverage Δ
e2e 40.81% <ø> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

LucianoPierdona
LucianoPierdona previously approved these changes Sep 1, 2022
@ggazzo ggazzo added the stat: ready to merge PR tested and approved waiting for merge label Sep 1, 2022
gabriellsh
gabriellsh previously approved these changes Sep 6, 2022
@ggazzo ggazzo added stat: needs QA and removed stat: ready to merge PR tested and approved waiting for merge labels Sep 8, 2022
@filipemarins filipemarins added stat: ready to merge PR tested and approved waiting for merge stat: QA tested and removed stat: needs QA labels Sep 8, 2022
@filipemarins filipemarins changed the title [FIX] Asset settings description not showing [FIX] Asset settings description not showing on admin Sep 8, 2022
@filipemarins filipemarins added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Sep 8, 2022
Copy link
Member

@ggazzo ggazzo left a comment

Choose a reason for hiding this comment

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

could you instead of using data-qa-something make use of role=button[name=something]? I think it's much better and it guarantees that we have semantic access to the buttons

@kodiakhq kodiakhq bot merged commit 5bfcc21 into develop Sep 15, 2022
@kodiakhq kodiakhq bot deleted the fix/assets branch September 15, 2022 15:25
@filipemarins filipemarins added this to the 5.2.0 milestone Sep 21, 2022
@tassoevan tassoevan mentioned this pull request Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

squad: team-collab stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants