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

reCAPTCHA: Text padding realignment #2540

Open
3 tasks done
Tracked by #2537
machikoyasuda opened this issue Nov 26, 2024 · 5 comments
Open
3 tasks done
Tracked by #2537

reCAPTCHA: Text padding realignment #2540

machikoyasuda opened this issue Nov 26, 2024 · 5 comments
Assignees
Labels
front-end HTML/CSS/JavaScript and Django templates

Comments

@machikoyasuda
Copy link
Member

machikoyasuda commented Nov 26, 2024

Must have

  • Go through each and every page, and confirm padding between text element are 24px (that's a p-4 or m-4 in Bootstrap class utility language) on both Figma and app.
  • Go through each and every page, and change the H1 to start from 72px (p-5 m-4) from the nav.

Nice to have

  • Go through styles.css to see if there are any 24px or 72pxs being called, replace with classes
  • Pages should NOT be delivering empty divs. Refactor base to not do this.
  • Combine unnecessary divs
  • Renaming confusing base stuff
  • Take out unnecessary blocks, use base blocks.

I recommend this ticket be completed last.

@machikoyasuda machikoyasuda changed the title Padding realignment: 24px between text elements. Start page at 72px from the nav. reCAPTCHA: Text padding realignment Nov 26, 2024
@thekaveman thekaveman added the front-end HTML/CSS/JavaScript and Django templates label Nov 26, 2024
@machikoyasuda
Copy link
Member Author

Since this is the last reCAPTCHA ticket, I'm going to be writing any notes in here of any other possible improvements/refactors we can now do:

  • Should we get rid of the Media Item includes entirely?
  • The app is full of this kind of pattern:
<div class="container">
  <div class="row justify-content-center">
      <div class="col-lg-X"></div>
  </div>
  <div class="row justify-content-center">
       <div class="col-lg-X"></div>
  </div>
  <div class="row justify-content-center">
      <div class="col-lg-X"></div>
  </div>
</div>

In the past, this was necessary because some elements had a different column width. But now, they are all col-lg-6. Can we simplify to:

<div class="container">
  <div class="row justify-content-center">
      <div class="col-lg-6">
         // everything goes in here
      </div>
  </div>
</div>

@machikoyasuda
Copy link
Member Author

machikoyasuda commented Dec 6, 2024

Probably can now remove: explanatory-text-wrapper and maybe even cta button from base.

Instead of:

        <div class="container main-row">
          <div class="row justify-content-center">
            {% block headline %}
            {% endblock headline %}
          </div>
          {% block explanatory-text-wrapper %}
            <div class="col-lg-8">
              {% block explanatory-text %}
              {% endblock explanatory-text %}
            </div>
          {% endblock explanatory-text-wrapper %}
          <div class="row justify-content-center">
            {% block inner-content %}
            {% endblock inner-content %}
          </div>
          {% block call-to-action %}
            <div class="row d-flex justify-content-center pt-8">
              <div class="col-12 col-lg-6">
                {% block call-to-action-button %}
                {% endblock call-to-action-button %}
              </div>
            </div>
          {% endblock call-to-action %}
        </div>

Can now just have:

        <div class="container main-row">
          <div class="row justify-content-center">
               <div class="col-12 col-lg-6">
                  {% block inner-content %}
                  {% endblock inner-content %}
               </div>
          </div>
        </div>

EXCEPT Enrollment Sucess is actually col-lg-9 or something weird and one-off.

@machikoyasuda
Copy link
Member Author

machikoyasuda commented Dec 6, 2024

Maybe keep call-to-action-button but collapse the others. The goal is to not have a bunch of empty divs on pages.

Actually I retract this statement. A misleading thing about the cta in base is that not all ctas use that (like on forms). So I think it makes sense to remove the cta from base too.

@machikoyasuda
Copy link
Member Author

The kind of refactor desired is achieved here: #2585

@machikoyasuda machikoyasuda self-assigned this Dec 11, 2024
@machikoyasuda machikoyasuda moved this from Todo to In progress in Digital Services Dec 11, 2024
@machikoyasuda
Copy link
Member Author

#2585 (comment)

@machikoyasuda machikoyasuda mentioned this issue Dec 12, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front-end HTML/CSS/JavaScript and Django templates
Projects
Status: In progress
Development

No branches or pull requests

3 participants