-
Notifications
You must be signed in to change notification settings - Fork 140
Theme: adding theme template #2390
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
Conversation
eaa72f5 to
d5acf8d
Compare
duboisp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'adore la restructuration que tu as fais du markup de la page. Good job.
Il va falloir ajouter une page d'exemple soit pour le dernier menu item ou soit pour un des avants derniers item du menu. Ceci afin de bien voir l'effet de la class "active" qui est utilisé dans le menu de gauche pour identifier l'item courant.
d5acf8d to
6dd44be
Compare
6dd44be to
01f3e24
Compare
01f3e24 to
9519454
Compare
duboisp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is that theme page should still have the "Top menu" button?
If not, then the "sign in" button for the "Jobs" theme example need to be at the same line of the breadcrumb. (I didn't tested that use case if that is already supported or not yet.)
|
@Garneauma can you add in the request description our internal reference number associated with this PR? thanks |
9519454 to
8506ffc
Compare
8506ffc to
d019dbd
Compare
d019dbd to
239e198
Compare
d9be7db to
6658ef0
Compare
6658ef0 to
5eecdec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review checklist:
- feature integration
- documentation - All good except the following:
- Check if instruction are specified about adding "active" css class to the corresponding menu item
- Revise the static text list to ensure it is complete
- If we have the info, update the DTO link to the guidance or point for now to their generic page
- code - Check with only the following to note:
- Concern raised about the element id
gridContainerwhen we will implement in MWS.
- Concern raised about the element id
- technical test - Completed and pass
- functional check - Completed, except
- Please add an explicit removal of the top menu. This can be a JS check/function. We don't know if actually the removal are going to be completed before this.
- versioning impact - This is a major change for the template, but a patch for GCWeb
- maintenance plan - Nothing to note
- Previous comment/concerns - Was all addressed
duboisp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Previous comment/concerns - Was all addressed
- Confirmed that the top menu are going to co-exist with this page.
- Functional check - completed En & FR
- documentation - lgtm
- code - Updated line was reviewed. If the
gridContainerbecome an issue when implementing in a new template 'Editable Template', we will address that later, for now it should work as expected.
We will be able to merge, once we have the ok from BA or/and DTO via our internal ticket.
|
@BeraJosh as discussed, for you review. A simple comment saying "all good" will be sufficient. Thanks. |
duboisp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependent on PR #2410 being merged.
Changes related to: WET-467