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

[#2036] Add tab-design for login page #992

Merged
merged 5 commits into from
Apr 19, 2024

Conversation

jiromaykin
Copy link
Contributor

@jiromaykin jiromaykin commented Jan 25, 2024

issue: https://taiga.maykinmedia.nl/project/open-inwoner/task/2036
Differs a bit in design from the tab we have in 'Mijn uitkeringen'

Log in options are now split in multiple tabs (but Login URL stays the same):
https://www.figma.com/file/iKGhWhstaLIlFSaND2q7cE/OIP---Designs-(new)?type=design&node-id=270-6788&mode=design&t=DpALupfBX7lYWPsm-0
Clicking on "Log in met e-mail adres" will show the hidden log-in form.

Direct link from the outside for business users is ".../accounts/login/#zakelijk" because the leading slash indicates the root of the URL path, and the hash anchor is appended to it.

Also added two new views:
- http://localhost:8000/accounts/login-business/
- http://localhost:8000/accounts/login-email/

Removed urls/views again.

@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.07%. Comparing base (8e638a5) to head (dfd16d9).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #992   +/-   ##
========================================
  Coverage    95.07%   95.07%           
========================================
  Files          955      955           
  Lines        33961    33961           
========================================
  Hits         32288    32288           
  Misses        1673     1673           

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

@jiromaykin jiromaykin force-pushed the feature/2036-implement-login-tab-design branch 4 times, most recently from 33c8fac to 1193387 Compare January 30, 2024 07:30
@jiromaykin jiromaykin added the on hold Pause working for now, continued after decisions label Feb 5, 2024
@jiromaykin jiromaykin force-pushed the feature/2036-implement-login-tab-design branch 4 times, most recently from 7fa8ef6 to 2c7ac45 Compare February 19, 2024 14:36
@jiromaykin jiromaykin force-pushed the feature/2036-implement-login-tab-design branch 3 times, most recently from 8cde779 to d838919 Compare March 12, 2024 18:47
@jiromaykin jiromaykin added wip Work in progress and removed on hold Pause working for now, continued after decisions labels Mar 14, 2024
@jiromaykin jiromaykin force-pushed the feature/2036-implement-login-tab-design branch from c043537 to 5477090 Compare March 14, 2024 16:52
@alextreme alextreme added the on hold Pause working for now, continued after decisions label Mar 18, 2024
@Bartvaderkin
Copy link
Contributor

Bartvaderkin commented Mar 18, 2024

Note: I tried fixing the tests but I'm not sure if we cover every login option (there are quite a few combinations of digid, eherkenning and admin with and without OIDC, plus the regular email login). For each case we need to test multiple situation (new user, updating user, existing email etc) but they get lost in boilerplate.

@jiromaykin jiromaykin removed the on hold Pause working for now, continued after decisions label Mar 21, 2024
@jiromaykin jiromaykin force-pushed the feature/2036-implement-login-tab-design branch 4 times, most recently from 6511d6c to d533c65 Compare April 9, 2024 14:53
@jiromaykin jiromaykin force-pushed the feature/2036-implement-login-tab-design branch 2 times, most recently from acaae89 to f774cab Compare April 18, 2024 07:48
@jiromaykin jiromaykin marked this pull request as ready for review April 18, 2024 07:53
@jiromaykin jiromaykin marked this pull request as draft April 18, 2024 08:50
@jiromaykin jiromaykin force-pushed the feature/2036-implement-login-tab-design branch from 54ab64a to c81eca4 Compare April 18, 2024 13:10
@jiromaykin jiromaykin removed the wip Work in progress label Apr 18, 2024
@jiromaykin jiromaykin force-pushed the feature/2036-implement-login-tab-design branch from be49c34 to 6c89004 Compare April 18, 2024 19:19
@jiromaykin jiromaykin force-pushed the feature/2036-implement-login-tab-design branch from b86d6f5 to dfd16d9 Compare April 18, 2024 20:53
@jiromaykin jiromaykin marked this pull request as ready for review April 18, 2024 21:37
@Bartvaderkin Bartvaderkin self-requested a review April 19, 2024 07:22
Comment on lines +54 to +57
'.tab__header[href="/accounts/login/#particulier"]'
)
const zakelijkLink = document.querySelector(
'.tab__header[href="/accounts/login/#zakelijk"]'
Copy link
Contributor

@Bartvaderkin Bartvaderkin Apr 19, 2024

Choose a reason for hiding this comment

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

Feedback for later: Django URL's are technically dynamic so we don't want hardcoded URL's in the Javascript (or HTML, CSS etc). This is why there are {% url "xyz" %} and {% static 'xyz' %} in the templates (to reverse() the URLs from whatever is in the urls.py's).

So these Link selectors should be different and not use the href=, but a class or id (possibly chain a .querySelector() from the Tab elements you grab by ID just below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, @Bartvaderkin I will have to create new id's/selectors here because the tabs are not inside those panels-with-id's;
they are elsewhere in the HTML structure.

Comment on lines +82 to +83
const loginformFocuses = document.querySelectorAll(LoginFormFocus.selector)
;[...loginformFocuses].forEach((element) => new LoginFormFocus(element))
Copy link
Contributor

@Bartvaderkin Bartvaderkin Apr 19, 2024

Choose a reason for hiding this comment

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

Feedback for later: The queryselector has a forEach() so you can just chain from that:

document.querySelectorAll(LoginFormFocus.selector).forEach((element) => new LoginFormFocus(element))

Comment on lines +54 to +55
const tabpanels = document.querySelectorAll(TabPanel.selector)
;[...tabpanels].forEach((tabpanel) => new TabPanel(tabpanel))
Copy link
Contributor

Choose a reason for hiding this comment

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

😉

window.addEventListener('load', () => {
const hash = window.location.hash
if (hash) {
const tabHeader = document.querySelector(`.tab__header[href="${hash}"]`)
Copy link
Contributor

@Bartvaderkin Bartvaderkin Apr 19, 2024

Choose a reason for hiding this comment

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

This doesn't work as the tabs have the path AND a hash as href now.

And in general I feel it is better to not select on href like this because it often has these issues with unexpected dependencies here or there. (see also the earlier note about LoginForm.js)

Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

I appended some fixes to the urls in login.html (the next url wasn't applied to both the text and icon url, and some urls were hardcoded).

I also added some comments for later (most just code style, one possible issue but it is not an important feature and doesn't seem to come up in normal use).

I also checked the conditionals so I think this is good to go for acceptance.

@alextreme alextreme merged commit a18f746 into develop Apr 19, 2024
15 checks passed
@alextreme alextreme deleted the feature/2036-implement-login-tab-design branch April 19, 2024 09:51
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.

4 participants