[WIP] [Don't Merge] Utilize frontend-component-header for studio header #69

brian-smith-tcril · 2022-09-27T15:30:56Z

This PR is a work in progress

todo list

use helper classes instead of making new ones where possible/appropriate

…uper wip)

…der will fix the error? (nope)

arbrandes

Excellent work, @brian-smith-tcril! I totally approve of the direction: I find the whole thing to be much more pleasant to navigate. To be perfectly honest, modulo removal of some commented code and a couple of console issues, I would consider this to be Almost Done ™️. Congrats!

Btw, I think it's a good idea to consolidate commits somewhat before merging. I'll leave it to you to pick locus themes.

Another note: there are some feature flag-hidden UI elements to this MFE that I haven't tested, yet, but since I know for a fact the underlying feature I'm thinking of is not actually working at the moment (LTI 1.3 support), I'll leave that to yet another issue to put in the backlog.

arbrandes · 2022-10-03T14:43:13Z

package.json

@@ -37,8 +37,9 @@
  "dependencies": {
    "@edx/brand": "npm:@edx/brand-openedx@1.1.0",
    "@edx/frontend-component-footer": "10.2.1",
-    "@edx/frontend-platform": "1.15.2",
-    "@edx/paragon": "19.25.3",
+    "@edx/frontend-component-header": "github:brian-smith-tcril/frontend-component-header#studio-header-for-prs",


Thanks for this. Makes it much easier to test.

Do you mind making a PR out of the frontend-component-header changes, though? However WIP it still is.

openedx/frontend-component-header#264

Excellent, thank you!

arbrandes · 2022-10-03T15:58:30Z

src/index.jsx

-      <div className="wrapper">
-        <StudioHeader />
-        <main>
+        {/* todo: look for a better way to do this, using these routes in


Interested in hearing suggestions for alternatives. I assume you're wrapping StudioHeaderWrapper in the Switch in order to clear the component's state? when on the home page?

my main issue with this implementation is having multiple route switches in the index.jsx file here

i did add it in f934987 to clear the state (since loadingStatus would stay loaded when navigating back to the list page)

i was hoping i'd have some better ideas by now, but nothing is coming to mind

For what it's worth, it seems to me like a legitimate use of Switch, and a reasonable place to put it in. 🤷🏼

sounds good, i'll remove the todo

arbrandes · 2022-10-03T16:15:38Z

src/index.scss

 @import '~@edx/frontend-component-footer/dist/footer';

 @import 'vendor/normalize.css';
 @import 'vendor/studio.scss';
 @import 'vendor/overrides.scss';

 @import './library-authoring/index.scss';
+
+// Breadcrumbs
+// todo: figure out why this var isn't working (and change it to a reasonable value)


I think we figured it out, right?

yup! removed in brian-smith-tcril@f048454

edit: actually removed in 79fb801

arbrandes · 2022-10-03T16:18:42Z

src/library-authoring/author-library/LibraryAuthoringPage.jsx

-    <Button variant="success" className="mr-1" size="lg" disabled={sending} onClick={quickAddBehavior}>
+    {/* todo: either reimplement the scroll to the add components button functionality,
+              figure out a better UX for the add component button at the top, or just
+              remove it entirely */}


Did you remove it as a UX consideration, or just to simplify the refactor temporarily?

from a UX stand point i think that button was bad. i'm perfectly happy with the solution being "just have the add component buttons at the bottom", but i can see how needing to scroll through a long list of blocks to get there could be frustrating.

i don't think having a "scroll me to the bottom" button labeled as a "add component" makes much sense

i guess my general UX feeling tl;dr here is: either we should:

only have the buttons on bottom
OR

only have a button on top (just move the existing bottom buttons functionality into a dropdown on top)

I'm fine with getting rid of the button, for now. In the end it won't be a decision to make on our own, but we can always add the button back if somebody complains.

(What will eventually happen, I expect, is that this page will suffer a major UX overhaul. Whoever develops the changes will likely get handed down a full design document.)

arbrandes · 2022-10-03T16:19:17Z

src/library-authoring/author-library/LibraryAuthoringPage.jsx

+            </ActionRow>
+            {/* todo: figure out how we want to handle these at low screen widths.
+                      mobile is currently unsupported: so it doesn't make sense
+                      to have partially implemented responsive logic */}


+1. As we discussed previously, mobile is not currently a priority for Studio-like MFEs such as this one.

arbrandes · 2022-10-03T16:32:33Z

src/library-authoring/course-import/CourseImportTaskListItem.jsx

+    //       <span className="value">{new Date(task.created_at).toLocaleString()}</span>
+    //     </span>
+    //   </div>
+    // </div>


Reminder to remove the reference material :)

addressed in 42c82d4

arbrandes · 2022-10-03T16:35:49Z

src/library-authoring/edit-block/LibraryBlockPage.jsx

-        <div className="wrapper-mast wrapper">
+        {/* todo: figure out what we want here, the header text always says "Component", and the 
+                  "back to library" button is redundant considering the header link with the
+                  library name goes back to the library */}


Agreed. I think it's fine to get rid of the redundancy.

removed brian-smith-tcril@9edd6f5

arbrandes · 2022-10-03T16:38:55Z

src/library-authoring/list-libraries/index.scss

+//       font-size: 1.4rem;
+//     }
+//   }
+// }


Reminder to get rid of the saved reference.

addressed in 42c82d4

arbrandes · 2022-10-03T16:41:35Z

src/library-authoring/studio-header-wrapper/ContentTitleBlock.jsx

+    <Link
+      className='content-title-block'
+      to={destination}
+      aria-label={ariaLabel}


Thanks for sticking with a11y. 👍🏼

(Disclaimer: I'm not doing a proper accessibility review here. This is going to be the topic of a separate discussion. Simply using more Paragon helps a lot, though.)

arbrandes · 2022-10-03T16:43:29Z

src/vendor/overrides.scss

+
+// todo: probably want to move this to the header component
+// but for now i'm just making changes here so the logos in
+// the header and the footer line up at every viewport width


Yeah, we definitely want to move stuff off of vendor. We can leave that for the next issue to tackle: #72

arbrandes · 2022-10-03T16:55:06Z

Two things I forgot to note above:

I tested everything I could and things Just Worked (kudos!)
We'll also want to fix the linting issues prior to merging, of course

brian-smith-tcril · 2022-10-05T13:46:39Z

src/library-authoring/author-library/LibraryAuthoringPage.jsx

+    <Card.Header
+      className='library-authoring-block-card-header'
+      title={block.display_name}
+      // subtitle={library.blockTypes.find(bt => bt.block_type == block.block_type).display_name}


@arbrandes this is a leftover from a thought i had about including the block type in the cards as the subtitle

i think it could be a nice addition, but it didn't exist before, maybe it's worth making an issue on the repo for?

This is one of those things that will have to be brought up in a UX/Design discussion with stakeholders. For now, I think the best place to keep track of these ideas is in issues in this repository: either all of them in a single one, or tracking them separately (but as subtasks of another).

made an issue https://github.com/openedx/frontend-component-header/issues/266, removed the commented line 720bc0c

brian-smith-tcril · 2022-10-05T13:53:04Z

src/library-authoring/author-library/LibraryAuthoringPage.jsx

+          <h1 className="page-header">{library.title}</h1>
+        </div>
+        <ActionRow.Spacer />
+        {/* todo: hide/show previews seems odd where it is right now, maybe change that? */}


this todo is putting it lightly, having a global show/hide previews button has caused the tab to crash for me when enabling all the previews at once and scrolling around.

i'm not sure if there's a good way to address that issue without any UX changes, and i'd be very happy if we could move to a "show preview per block" UX instead

As mentioned above, let's track this. It's definitely worth its own issue, since it has clear adverse effects - even if it's not the easiest to reproduce.

made an issue, https://github.com/openedx/frontend-component-header/issues/265, removed the comment fbef590

brian-smith-tcril · 2022-10-05T14:12:26Z

src/library-authoring/author-library/LibraryAuthoringPage.jsx

-        )}
-        <Link to={editView}>
-          <Button size="lg" className="mr-1">
+  <Card className='w-auto m-2'>


using m-2 here makes it feel a little cramped on the list in my opinion, but adding mb-3 makes it feel a little too spaced out. i prefer m-2, and i'd rather stick to using bootstrap helper classes for this if possible

No objections at all.

brian-smith-tcril · 2022-10-05T14:24:27Z

src/library-authoring/author-library/index.scss

+.library-authoring-sidebar {
+    font-size: 90%;
+    h3 {
+      font-size: 100%;
+      font-weight: bold;
+    }


tried using fs-* helper classes but those might be only available in a newer version of bootstrap? keeping this class for now

brian-smith-tcril · 2022-10-05T16:24:20Z

src/library-authoring/library-access/LibraryAccessPage.jsx

+              {/* todo: figure out if we want this, we already have the "New Team Member" button at the top, having a
+                        "Add User" button with identical functionality feels redundant */}
+              {/* {isAdmin && (
+                <div className="well mt-3">
+                  <Row className="h-100">
+                    <Col xs={12} md={8} className="my-auto">
+                      <h2 className="h2 font-weight-bold">{intl.formatMessage(messages['library.access.well.title'])}</h2>
+                      <p>{intl.formatMessage(messages['library.access.well.text'])}</p>
+                    </Col>
+                    <Col xs={12} md={4} lg={3} className="my-auto offset-lg-1 text-center text-md-right">
+                      <Button variant="success" size="lg" onClick={() => setShowAdd(true)}>
+                        <FontAwesomeIcon icon={faPlus} className="pr-1 icon-inline" />
+                        <strong>{intl.formatMessage(messages['library.access.well.button'])}</strong>
+                      </Button>
+                    </Col>
+                  </Row>
+                </div>
+              )} */}


i'd like to just remove this because of redundancy, is that cool?

Please do! I know for a fact this page was whipped up with no UX supervision, so we have some leeway.

removed in e38d674

brian-smith-tcril · 2022-10-05T16:57:04Z

package.json

@@ -37,8 +37,9 @@
  "dependencies": {
    "@edx/brand": "npm:@edx/brand-openedx@1.1.0",
    "@edx/frontend-component-footer": "10.2.1",
-    "@edx/frontend-platform": "1.15.2",
-    "@edx/paragon": "19.25.3",
+    "@edx/frontend-component-header": "github:brian-smith-tcril/frontend-component-header#studio-header-for-prs",


openedx/frontend-component-header#264

brian-smith-tcril · 2022-10-21T13:05:41Z

implemented in #74

Brian Smith added 30 commits September 13, 2022 14:51

use studio header from component

919aad7

comment out all styles that don't break the build when removed

6fa0d23

use logo from env

2b86193

upgrade dependencies, rebuild package-lock

305b252

try overriding stuff all over the place

1169f64

not sure how that got checked in

5044fca

remove wrappers etc

d8c484d

line up header/footer logos

9d35b9c

margin-top for main content, use actionrow for new lib button

2946b63

better utilize paragon card functionality for library list page

eacec82

• instead

d18cabd

add some flex stuff

a9f90c1

trying to fill the height

62abab5

full height

a447a5c

add breadcrumbs to authoring page

72703ee

start move to actionrow for lib authoring page

8fad5bf

button cleanup

28a6190

add todo for bringing back help link

7b84c3f

add lib settings dropdown to actionrow

a1c830e

settings dropdown stuff

587bbfa

messages for settings dropdown

f62ec7e

breadcrumbless

7af6c5a

wip stuff (i know i shouldn't check in module.config.js but this is s…

5dcf86a

…uper wip)

add wrapper, refactor to use refactored studio header component

c8e4496

move actionrowcontent to a spot where we can feed it data

947bafe

start doing basic data stuff

c1c3e85

get the lib title in the header!

8341d60

move settings dropdown into header, update contenttitle link

38d77af

add todos for next steps

2479fde

maybe updating react-router-dom to match the version in component hea…

847d028

…der will fix the error? (nope)

arbrandes assigned brian-smith-tcril Sep 30, 2022

arbrandes linked an issue Sep 30, 2022 that may be closed by this pull request

Use a shared header component, instead of a custom one. #70

Closed

arbrandes requested changes Oct 3, 2022

View reviewed changes

Brian Smith added 14 commits October 4, 2022 10:25

remove testing css

f048454

fix react key errors

ff8d45a

fix missing key error

b56b7d2

remove redundancy

9edd6f5

remove library metadata div/css

42c82d4

use helper class for margin bottom instead

1263b90

use helper classes for lib list page

866d08a

remove unused breadcrumb testing css

79fb801

use helper classes for component search/filter actionbar

2b8d650

use helper classes for block cards

37b5cc3

remove commented out styles

1549782

remove unused indextabs component and unused styles

71f1e47

remove testing/unused styles from vendor overrides

ee12c7d

todone

e0f4a5e

brian-smith-tcril mentioned this pull request Oct 5, 2022

[WIP] [Don't Merge] reusable studio header component openedx/frontend-component-header#264

Closed

brian-smith-tcril commented Oct 5, 2022

View reviewed changes

Brian Smith added 4 commits October 5, 2022 14:24

Merge branch 'main' into use-frontend-component-header

744d97d

remove redundant commented out code

e38d674

remove comment about show/hide previews

fbef590

remove commented line

720bc0c

brian-smith-tcril mentioned this pull request Oct 6, 2022

refactor: use frontend-component-header, move away from old studio styles #74

Merged

brian-smith-tcril closed this Oct 21, 2022

brian-smith-tcril deleted the use-frontend-component-header branch October 21, 2022 13:05

arbrandes linked an issue Oct 31, 2022 that may be closed by this pull request

CSS refactor #72

Closed

4 tasks

arbrandes removed a link to an issue Oct 31, 2022

CSS refactor #72

Closed

4 tasks

[WIP] [Don't Merge] Utilize frontend-component-header for studio header #69

[WIP] [Don't Merge] Utilize frontend-component-header for studio header #69

Conversation

brian-smith-tcril commented Sep 27, 2022 • edited

This PR is a work in progress

todo list

arbrandes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-smith-tcril Oct 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbrandes commented Oct 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-smith-tcril commented Oct 21, 2022

brian-smith-tcril commented Sep 27, 2022 •

edited

brian-smith-tcril Oct 4, 2022 •

edited