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

CBIGR Terms of Use #11

Merged
merged 1 commit into from
Oct 26, 2020

Conversation

laemtl
Copy link

@laemtl laemtl commented Oct 19, 2020

  • add a Terms of Use modal
  • the modal displays on the first visit
  • a menu item can display the modal at any time

Screenshot from 2020-10-19 15-50-18

@@ -831,3 +831,8 @@ legend {
font-size: 14px;
color: #064785;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@HenriRabalais do we not already have css for modals ?

Copy link
Author

@laemtl laemtl Oct 19, 2020

Choose a reason for hiding this comment

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

Without this change the modal does not have a scroll bar. I think a scroll bar is useful because this modal does have a lot of text so the Accept button is hidden otherwise.

Copy link
Collaborator

Choose a reason for hiding this comment

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

then my question becomes, is this gonna affect the modals in the biobank ??? and im pretty sure those have scrolling

Copy link
Author

@laemtl laemtl Oct 19, 2020

Choose a reason for hiding this comment

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

Screenshot from 2020-10-19 16-18-16

Copy link
Owner

@HenriRabalais HenriRabalais Oct 19, 2020

Choose a reason for hiding this comment

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

@ridz1208 yeah, the modal windows in the Biobank does have an overflow: scroll property. This is styled directly into the jsx component though and is not done through the css class structure.

Screen Shot 2020-10-19 at 16 52 41

Copy link
Collaborator

Choose a reason for hiding this comment

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

how come its not working for @laemtl ??

Copy link
Owner

Choose a reason for hiding this comment

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

@ridz1208 because this PR doesn't make use of the jsx/Modal.js component.

Copy link
Author

@laemtl laemtl Oct 20, 2020

Choose a reason for hiding this comment

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

I can take care of the reactification once the priorities have been dealt with. For now, I have integrated my style changes directly into the html to avoid conflicts with the react component.

@laemtl laemtl force-pushed the 2020-10-19-terms-uses branch from c703da3 to 546fa40 Compare October 19, 2020 20:15
@ridz1208
Copy link
Collaborator

@laemtl this seems to be doing something that could easily be reactified and turned into an override... I will leave it as is for now cause better having this than nothing

This PR should not be merged into this Branch a new branch will be created for the open version

@laemtl
Copy link
Author

laemtl commented Oct 19, 2020

@ridz1208 Do you have some potential ideas to turn this into an override? Is there a system in LORIS to add a widget into all pages?

@ridz1208
Copy link
Collaborator

@laemtl maybe adding it to the request account logic (instead of on first visit, you would have to agree to the terms when you request your user... but its not a priority now)

@laemtl
Copy link
Author

laemtl commented Oct 19, 2020

One problem I see with this approach is that once agreed, the Terms of Use can't be accessed anymore.

@ridz1208
Copy link
Collaborator

true, the other potential issue is if we go OPEN for real, there will be no more request account. thats why for now lets leave it as is

@laemtl laemtl force-pushed the 2020-10-19-terms-uses branch from 546fa40 to 56416d8 Compare October 20, 2020 17:10
@laemtl laemtl force-pushed the 2020-10-19-terms-uses branch from 56416d8 to 2277664 Compare October 20, 2020 17:16
@ridz1208 ridz1208 changed the base branch from Biobank to Biobank-open October 25, 2020 15:41
@ridz1208 ridz1208 merged commit 3f7bfa5 into HenriRabalais:Biobank-open Oct 26, 2020
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.

3 participants