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

Style calculators constructor show page #984

Conversation

olexandervanzuriak
Copy link
Contributor

@olexandervanzuriak olexandervanzuriak commented Nov 26, 2024

#963

Code reviewers

Summary of issue

All calculators should be in the same style as the diaper and menstrual hygiene calculator

  • the same colors by default
  • the same font by default

image

Result

Video1.mp4

Testing approach

Tested locally

CHECK LIST

  • СI passed
  • Сode coverage >=95%
  • PR is reviewed manually again (to make sure you have 100% ready code)
  • All reviewers agreed to merge the PR
  • I've checked new feature as logged in and logged out user if needed
  • PR meets all conventions

Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.31%. Comparing base (9130d91) to head (e144e42).
Report is 48 commits behind head on calculators-constructor.

Additional details and impacted files
@@                     Coverage Diff                     @@
##           calculators-constructor     #984      +/-   ##
===========================================================
+ Coverage                    83.98%   85.31%   +1.33%     
===========================================================
  Files                           65       65              
  Lines                          974      974              
===========================================================
+ Hits                           818      831      +13     
+ Misses                         156      143      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@olexandervanzuriak olexandervanzuriak self-assigned this Nov 27, 2024
<div class="container place-items-center">
<h1 class="text-4xl font-bold">Calculator <%= @calculator.name %></h1><br>
<%# TODO: Delete this and use user provided value %>
<% color = "blue" %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

колір має передаватися хексом

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix

<% calculator_image = "scales.png" %>

<div class="rounded jumbotron jumbotron-fluid position-relative">
<h1 class="pt-6 text-2xl font-semibold text-center font-sans text-<%= color %>">Calculator <%= @calculator.name %> <%= @text %></h1>
Copy link
Collaborator

Choose a reason for hiding this comment

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

не гуд. додай в конфіги color: { dynamic: "var(--calculator-color)" }, щось таке, тоді тут вже будеш десь зверху сетати цю змінну: <div style="--calculator-color: <%= @calculator.color %>">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix

Comment on lines 10 to 17
safelist: [
{
pattern: /bg-.*/,
},
{
pattern: /text-.*/,
},
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

це відповідно видалиш

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted

@@ -1,4 +1,6 @@
class Calculators::CalculationService
include ApplicationHelper
Copy link
Collaborator

Choose a reason for hiding this comment

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

не гуд

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted

Comment on lines 15 to 19
if current_locale?(:en)
{ label: formula.en_label, result: result, en_unit: formula.en_unit }
else
{ label: formula.uk_label, result: result, en_unit: formula.uk_unit }
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

тут ти можеш додати локальзацію додним рядком грубо кажучи:

{ label: formula.public_send("#{I18n.locale}_label") }

Copy link
Collaborator

Choose a reason for hiding this comment

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

та навіть прсотіше того, бо мав би бути підключений до формули концерн транслейтебел (він сам в залежності від локалі бере потрібний переклад):

{ label: formula.label }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix

<div class="description-content-box">
<p data-type="description_block_html" class="description-text">
<%# TODO: Change description text and links %>
Lorem ipsum dolor sit amet, consectetur adipiscing elit. Phasellus faucibus arcu iaculis placerat euismod. Pellentesque nibh arcu, varius ac arcu a, eleifend semper leo. Aliquam ultricies lacus at mi ornare, sit amet vestibulum est fermentum. Praesent sed dapibus elit. Curabitur congue ante non metus posuere porttitor. Etiam non lacus imperdiet turpis pellentesque viverra. Nulla in velit mi. Suspendisse molestie tempor ornare.
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. забери текст
  2. Додай умову, щоб не показувати секцію, поки нема контенту (просто if false)
  3. залиш тік div, бо контент едітора, який потім додадуть, буде містити багато інших тегів і так буде правельніше

Copy link
Collaborator

Choose a reason for hiding this comment

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

має бути просто div, в який потім передадуть контент одною строкою

Comment on lines 3 to 5
<div>
<%# TODO: Add description partial here %>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

а чому ти всі стилі видалив? ти мав залишити весь той контейнер, просто без контенту

Comment on lines 2 to 7
<% color = "#088F8F" %>

<%# TODO: Delete this and use user provided value%>
<% formula_image = "money_to_spent_2.png" %>

<div style="--calculator-color: <%= color %>" class="calculation-results rounded jumbotron jumbotron-fluid position-relative">
Copy link
Collaborator

Choose a reason for hiding this comment

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

а навіщо тут також колір передавати? він зверху передається та й все, ну на шоу пейджі

</div>
<% end %>
<%= image_tag "#{calculator_image}", class: "scales_img", alt: "Scales" %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

а навіщо ти в стрінгу огортаєш змінну?

</div>
<% end %>
<%= image_tag "#{calculator_image}", class: "scales_img", alt: "Scales" %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

alt через I18n пиши

Comment on lines 9 to 12
<%= image_tag "#{formula_image}", class: "img-margin", alt: "icon" %>
<p class="dynamic-text-color"><%= result[:result] %></p>
<p class="text-2xl dynamic-text-color"><%= result[:unit] %></p>
<p class="diapers-font-text"><%= result[:label] %></p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

вирівняй

Comment on lines 5 to 12
<div class="description-content-box">
<p data-type="description_block_html" class="description-text">
<%# TODO: Add description text and links %>
</p>
</div>
<div class="description-btn-box">
<%# TODO: Add discription btn link here %>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

має бути один ендпоінт, куди буде закидатися контент, ну я ж казав про це

Copy link
Collaborator

Choose a reason for hiding this comment

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

засинкайся з Сашею, вона тобі покаже, що на виході має, ви маєте законектитись

@olexandervanzuriak olexandervanzuriak merged commit 679ae88 into calculators-constructor Nov 29, 2024
5 checks passed
@olexandervanzuriak olexandervanzuriak deleted the 963-view-of-the-constructors-calculator branch November 29, 2024 16:23
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