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

Mithril 2 update #93

Merged
merged 28 commits into from
Sep 24, 2020
Merged

Mithril 2 update #93

merged 28 commits into from
Sep 24, 2020

Conversation

askvortsov1
Copy link
Member

No description provided.

Copy link
Member

@clarkwinkelmann clarkwinkelmann left a comment

Choose a reason for hiding this comment

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

I noticed some files don't have an ending newline. I suppose our CI should auto-format that?

js/src/admin/components/EditTagModal.js Show resolved Hide resolved
js/src/admin/components/TagsPage.js Outdated Show resolved Hide resolved
js/src/forum/addTagList.js Outdated Show resolved Hide resolved
js/src/forum/components/TagDiscussionModal.js Show resolved Hide resolved
js/src/forum/components/TagDiscussionModal.js Outdated Show resolved Hide resolved
Copy link
Member

@clarkwinkelmann clarkwinkelmann left a comment

Choose a reason for hiding this comment

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

Looking good code-wise

@@ -23,7 +23,7 @@ export default class TagsPage extends Page {
<div className="TagsPage">
{IndexPage.prototype.hero()}
<div className="container">
<nav className="TagsPage-nav IndexPage-nav sideNav" config={IndexPage.prototype.affixSidebar}>
<nav className="TagsPage-nav IndexPage-nav sideNav">
Copy link
Contributor

Choose a reason for hiding this comment

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

No replacement for the affixSidebar stuff? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

On the TagsPage, the navbar is at the top, and is absolutely unaffixed. I believe I removed this because it didn't actually do anything (and that method was removed from IndexPage in core)

config(...args) {
super.config(...args);
oncreate(vnode) {
super.oncreate(vnode);
Copy link
Contributor

Choose a reason for hiding this comment

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

No onupdate stuff here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Page title and title count doesn't change while on TagsPage. If it does changed (we just switched from a different page), oncreate will run. So onupdate shouldn't be needed.


app.modal.close();

m.redraw.strategy('none');
Copy link
Contributor

Choose a reason for hiding this comment

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

I am missing an e.redraw = false here. But this change is confusing anyway. Oversight from applying modal state changes to all bundled extensions?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a workaround from before when app.modal.close was broken. That being said, I kind of like how it is now, directly calling this.hide() instead of calling it through the global ModalState

Copy link
Contributor

Choose a reason for hiding this comment

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

But doing so does not mark the modal as closed in the state, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, if that's an issue here, that also affects closing modals by clicking (x), as that also calls this.hide().

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps we should build in something to clear the modal without a redraw in ModalManager's animateHide?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, fair point. I guess we can live with it here, but both cases should be fixed at some point, as they would both mean state and view getting out of sync?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, I'll make an issue. It doesn't seem to affect the UI, but it's still something that would be preferable to fix.

@askvortsov1 askvortsov1 merged commit 4a66095 into master Sep 24, 2020
@franzliedke franzliedke deleted the mithril-2-update branch September 25, 2020 08:03
askvortsov1 added a commit that referenced this pull request Mar 11, 2022
Update for Mithril 2

- TagLinkButtons now have children passed in, even though those children are not directly shown. This is because those children are used if that TagLinkButton is the active element in a dropdown.
- Since `m.redraw.strategy('all')` is no longer an option, we use keys to force a full-page rerender after rearranging tag order in the admin dashboard
askvortsov1 added a commit that referenced this pull request May 10, 2022
Update for Mithril 2

- TagLinkButtons now have children passed in, even though those children are not directly shown. This is because those children are used if that TagLinkButton is the active element in a dropdown.
- Since `m.redraw.strategy('all')` is no longer an option, we use keys to force a full-page rerender after rearranging tag order in the admin dashboard
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