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

feat: Update website design (GH-46) #48

Merged
merged 48 commits into from
Mar 10, 2022

Conversation

BlueSlug
Copy link
Member

@BlueSlug BlueSlug commented Feb 11, 2022

  • This pull request has been linted by running npm run lint without errors
  • This pull request has been tested by running npm run start and reviewing affected routes
  • This pull request has been built by running npm run build without errors
  • This isn't a duplicate of an existing pull request

Description

This update implements the ILDH site redesign described in this Figma file

This replaces the previous Eleventy conversion with a structure based on Trivet, and it replaces Stylus with Sass as the CSS preprocessor. Additionally, the Sass 7-1 directory structure and BEM have been used to organize styling rules.

This update represents a work in progress, so further issues may need to be filed to complete the work of implementing the design to its exact specifications.

Steps to test

Access the site via one of the following two options

  1. Run npm ci to install dependencies
  2. Run npm start to start the local server
  3. Access the site at http://localhost:8080/

Please note that links to previous file locations (ending in .html) will not work locally due to the _redirects file only functioning as intended when deployed via Netlify. All site content is served, though links have yet to be updated in all content.

OR

  1. Access the deploy preview site: https://deploy-preview-48--floe-handbook.netlify.app/

Additional information

Some remaining issues can be found on the Fluid Project Weekly Planning wiki page.

Related issues

Resolves #46

@netlify
Copy link

netlify bot commented Feb 15, 2022

✔️ Deploy Preview for floe-handbook ready!

🔨 Explore the source changes: 90da9eb

🔍 Inspect the deploy log: https://app.netlify.com/sites/floe-handbook/deploys/6229b50ebbc53200096ea5e2

😎 Browse the preview: https://deploy-preview-48--floe-handbook.netlify.app

@@ -0,0 +1,16 @@
name: Publish release
Copy link
Member

Choose a reason for hiding this comment

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

Is .github/workflows/release-please.yml still needed? The build process defined in the netlify.com for this site should trigger the auto-deploy of the website when there's a push to the main branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given that the Netlify settings for this site specify the image and command to use for building the site, it seems you're correct that we don't need to keep this action

@michelled
Copy link
Member

Bug: At certain widths the nav breaks onto a second line despite there being whitespace to the left of it.
ILDH Nav

It has been archived and can be found here:
<https://github.com/fluid-project/2015-handbook.floeproject.org>

## Working with Netlify CMS
Copy link
Member

Choose a reason for hiding this comment

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

This section can be removed when you remove the admin related doc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've opted to keep the section and add a note explaining how to restore this functionality. Do you think it would be better to remove it entirely?

Copy link
Member

Choose a reason for hiding this comment

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

Let's go with your idea. Keeping this section to explain how to restore this functionality will be helpful.

package.json Outdated
"build:eleventy": "cross-env NODE_ENV=production eleventy",
"clean": "rimraf dist",
"lint": "grunt lint",
"cms": "netlify-cms-proxy-server",
Copy link
Member

Choose a reason for hiding this comment

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

Dependencies on netlify-cms-proxy-server can be removed too along with the removal of the Netlify CMS admin code.

{% uioScripts %}
{% include "partials/global/scripts.njk" %}
</head>
{% block pageBody %}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment to explain the home page uses this pageBody block to define its own body layout?

@BlueSlug
Copy link
Member Author

BlueSlug commented Mar 9, 2022

Bug: At certain widths the nav breaks onto a second line despite there being whitespace to the left of it. ILDH Nav

@michelled: I've found a fix for this, though things start overlapping once UIO text size is increased. I'll address that next!

@BlueSlug BlueSlug marked this pull request as ready for review March 9, 2022 20:56
@cindyli
Copy link
Member

cindyli commented Mar 9, 2022

The layout of the "Narrations and Text-to-Speech" page is broken. Moreover, there are two "Further reading" sections on this page even though they use different heading levels.

@cindyli
Copy link
Member

cindyli commented Mar 9, 2022

The layout of the "WAI-ARIA" page is not right too. It's probably because <code> section contains code in long width and doesn't leave much space to the side content.

From my experiment, controlling the proportion of the content and the side content doesn't solve the problem unless the code in long width breaks into multiple lines. This might worth a discussion with designers.

@BlueSlug
Copy link
Member Author

BlueSlug commented Mar 9, 2022

The layout of the "Narrations and Text-to-Speech" page is broken. Moreover, there are two "Further reading" sections on this page even though they use different heading levels.

Good catch! I believe the broken layout is due to the pre and code elements not wrapping their contents. I'll evaluate my options, though quick fixes don't seem to be working

The repetition of the "further reading" heading is a content issue that predates my work. Should we change this?

@@ -0,0 +1,13 @@
<nav class="pagination" aria-label="pagination">
Copy link
Member

Choose a reason for hiding this comment

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

pagination.njk doesn't seem to be used anywhere. Can you double check? If it doesn't, please remove. Thanks.

@cindyli
Copy link
Member

cindyli commented Mar 9, 2022

The "Perspectives" title stretches out of the iphone 5 screen. Below is how it looks with the normal font size. Don't mention how much it will break the page when UIO font size increases. We probably should consider to apply the auto word wrap with hyphens to <body> element.

Screen Shot 2022-03-09 at 4 25 36 PM

@cindyli
Copy link
Member

cindyli commented Mar 9, 2022

Resources listed on the "Resources" section of "Meet Learner Needs and Preferences" page are categorized into "Text Simplification" and "Other". These category text are not showing on the side content. Is this expected behavior?


## Scripts

By default, the Mix [configuration file](../../webpack.mix.js) includes configuration for processing the three
Copy link
Member

Choose a reason for hiding this comment

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

There aren't three javascript files in /src/assets/scripts. It would be better to use more general wording. Thanks.

@cindyli
Copy link
Member

cindyli commented Mar 9, 2022

src/assets/README.md contains useful information. Could you add a description that points to this file from the README in the root directory so it doesn't get ignored? Thanks.

@cindyli cindyli merged commit e600ce2 into fluid-project:dev Mar 10, 2022
@cindyli
Copy link
Member

cindyli commented Mar 10, 2022

The content in the <code> block overlaps with the side menu. You probably wanna update the source .md file to manually break long lines as what you did for the second <code> block.

Screen Shot 2022-03-10 at 9 13 49 AM

@cindyli
Copy link
Member

cindyli commented Mar 10, 2022

Gradually increase UIO font size, you will notice a few issues:

On the desktop view:

  1. At some point, the menu bar at the top right will overlap with the logo at the top left;
  2. Go to a page having the side menu, such as the "Perspectives" page, the main content overflows and stretches out of the screen width. The content in the left panel even gets cut off and loses some content.

Screen Shot 2022-03-10 at 9 17 08 AM

On the mobile view:

  1. The menu icon doesn't overlap with the logo but the full width of these two elements exceeds the screen size causing the overflow.
  2. The page title, "Perspectives" in this case, does wrap into multiple lines. But these lines start to overlap on each other.

Screen Shot 2022-03-10 at 9 22 37 AM

Instead of using UIO, if you use "Command" and "+" to increase font size, the browser will handle the font change quite nicely: the browser translates the font change to the viewport size change so that when the font size is large enough, the viewport will be switched from the desktop view to the mobile view. UIO doesn't have this ability yet.

WeCount site ran into the same issue. The decision was to remove the text size change from UIO.

@cindyli
Copy link
Member

cindyli commented Mar 10, 2022

The content in the <code> block overlaps with the side menu. You probably wanna update the source .md file to manually break long lines as what you did for the second <code> block.

I just noticed the issue with the <code> block gets worse with the mobile view. Adding manual line breaks may not be a good solution. A more general solution might be showing code blocks in individual scrollable containers.

Screen Shot 2022-03-10 at 9 39 22 AM

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